Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

On 27/11/15 14:09, Richard Biener wrote:
> On Fri, Nov 27, 2015 at 10:44 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
> > On 26/11/15 16:54, Kyrill Tkachov wrote:
> > > On 26/11/15 16:49, Bernd Schmidt wrote:
> > > > On 11/26/2015 05:45 PM, Kyrill Tkachov wrote:
> > > > >           that doesn't help, punt.  */
> > > > >
> > > > > -  modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
> > > > >      if (tmp_b && then_bb)
> > > > >        {
> > > > These bits I thought would be part of a followup patch (which would also
> > > > guard against single_set problems), and as I mentioned I'd rather have a
> > > > checking assert.
> > > Yes, you're right. I have the checking_assert statement in the followup
> > > that I've been testing.
> > > I'll move the deletion of these two statements there as well to minimise
> > > the changes to this patch.
> > >
> > > I'll move these bits to that patch, re-build cc1 and commit.
> > Here it is.
> > I'm committing this to trunk.
> I think this causes
>
> FAIL: gcc.c-torture/execute/20050124-1.c   -O2  (internal compiler error)
> FAIL: gcc.c-torture/execute/20050124-1.c   -O2  (test for excess errors)
> WARNING: gcc.c-torture/execute/20050124-1.c   -O2  compilation failed to produce
>   executable
> FAIL: gcc.c-torture/execute/20050124-1.c   -O2 -flto -fno-use-linker-plugin -flt
> o-partition=none  (internal compiler error)
> FAIL: gcc.c-torture/execute/20050124-1.c   -O2 -flto -fno-use-linker-plugin -flt
> o-partition=none  (test for excess errors)
> ....

Sorry for that.
That is caused not by this patch but rather by the followup
https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html

The checking assert fails:
gcc_checking_assert (!emit_a || !modified_in_p (orig_b, emit_a));
emit_a is:
(parallel [
        (set (reg:SI 93)
            (plus:SI (reg/v:SI 88 [ i ])
                (const_int 2 [0x2])))
        (clobber (reg:CC 17 flags))
    ])

and and orig_b is:
(if_then_else:SI (eq (reg:CC 17 flags)
        (const_int 0 [0]))
    (reg/v:SI 87 [ <retval> ])
    (reg/v:SI 88 [ i ]))

So I think our assumption that this case would never trigger by this point doesn't hold
due to the CC reg clobber.
So the code before that patch was probably correct.
I think we should revert https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html then.

Kyrill

> /space/rguenther/src/svn/trunk2/gcc/testsuite/gcc.c-torture/execute/20050124-1.c:19:1:
> internal compiler error: in noce_try_cmove_arith, at ifcvt.c:2180^M
> 0x11f919d noce_try_cmove_arith^M
>          /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:2180^M
> 0x11fb93f noce_process_if_block^M
>          /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:3525^M
> 0x11fdd0e noce_find_if_block^M
>          /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:3974^M
> 0x11fdd0e find_if_header^M
>          /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:4179^M
> 0x11fdd0e if_convert^M
>          /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:5326^M
> 0x11ff32d execute^M
>
>
> on x86_64 with -m64 and -m32.
>
> Richard.
>
> > Thanks,
> > Kyrill
> >
> > 2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> >
> >      PR rtl-optimization/68506
> >      * ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block
> >      first if emit_a exists or then_bb modifies 'b'.  Reindent if-else
> >      blocks.
> >
> > 2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> >
> >      PR rtl-optimization/68506
> >      * gcc.c-torture/execute/pr68506.c: New test.
> >
> > > Thanks for your guidance,
> > > Kyrill
> > >
> > > > So take these deletions out and leave them for the followup, and the
> > > > patch is ok everywhere. No need for a full retest given that practically the
> > > > same patch has been tested already, just make sure you can build cc1.
> > > >
> > > >
> > > > Bernd