how does vrp2 rearrange this?

how does vrp2 rearrange this?

[Andrew MacLeod]

using testcase ifcvt-4.c:


typedef int word __attribute__((mode(word)));

word
foo (word x, word y, word a)
{
   word i = x;
   word j = y;
   /* Try to make taking the branch likely.  */
   __builtin_expect (x > y, 1);
   if (x > y)
     {
       i = a;
       j = i;
     }
   return i * j;

The current VRP2 pass takes:

   if (x_3(D) > y_4(D))
     goto <bb 3>; [50.00%]        <<---  note the THEN target
   else
     goto <bb 4>; [50.00%]
;;    succ:       3 [50.0% (guessed)]  count:536870912 (estimated 
locally) (TRUE_VALUE,EXECUTABLE)
;;                4 [50.0% (guessed)]  count:536870912 (estimated 
locally) (FALSE_VALUE,EXECUTABLE)

;;   basic block 3, loop depth 0, count 536870912 (estimated locally), 
maybe hot
;;    prev block 2, next block 4, flags: (NEW, REACHABLE, VISITED)
;;    pred:       2 [50.0% (guessed)]  count:536870912 (estimated 
locally) (TRUE_VALUE,EXECUTABLE)
;;    succ:       4 [always]  count:536870912 (estimated locally) 
(FALLTHRU,EXECUTABLE)

;;   basic block 4, loop depth 0, count 1073741824 (estimated locally), 
maybe hot
;;    prev block 3, next block 1, flags: (NEW, REACHABLE, VISITED)
;;    pred:       2 [50.0% (guessed)]  count:536870912 (estimated 
locally) (FALSE_VALUE,EXECUTABLE)
;;                3 [always]  count:536870912 (estimated locally) 
(FALLTHRU,EXECUTABLE)
   # i_1 = PHI <x_3(D)(2), a_5(D)(3)>
   # j_2 = PHI <y_4(D)(2), a_5(D)(3)>
   _6 = i_1 * j_2;
   # VUSE <.MEM_7(D)>
   return _6;

and turns it into :

;;   basic block 2, loop depth 0, count 1073741824 (estimated locally), 
maybe hot
;;    prev block 0, next block 3, flags: (NEW, REACHABLE, VISITED)
;;    pred:       ENTRY [always]  count:1073741824 (estimated locally) 
(FALLTHRU,EXECUTABLE)
   if (x_3(D) > y_4(D))
     goto <bb 4>; [50.00%]        <<-- has been reversed.
   else
     goto <bb 3>; [50.00%]
;;    succ:       4 [50.0% (guessed)]  count:536870912 (estimated 
locally) (TRUE_VALUE,EXECUTABLE)
;;                3 [50.0% (guessed)]  count:536870912 (estimated 
locally) (FALSE_VALUE,EXECUTABLE)

;;   basic block 3, loop depth 0, count 536870912 (estimated locally), 
maybe hot
;;    prev block 2, next block 4, flags: (NEW, VISITED)
;;    pred:       2 [50.0% (guessed)]  count:536870912 (estimated 
locally) (FALSE_VALUE,EXECUTABLE)
;;    succ:       4 [always]  count:536870912 (estimated locally) 
(FALLTHRU,EXECUTABLE)

;;   basic block 4, loop depth 0, count 1073741824 (estimated locally), 
maybe hot
;;    prev block 3, next block 1, flags: (NEW, REACHABLE, VISITED)
;;    pred:       3 [always]  count:536870912 (estimated locally) 
(FALLTHRU,EXECUTABLE)
;;                2 [50.0% (guessed)]  count:536870912 (estimated 
locally) (TRUE_VALUE,EXECUTABLE)
   # i_1 = PHI <x_3(D)(3), a_5(D)(2)>
   # j_2 = PHI <y_4(D)(3), a_5(D)(2)>
   _6 = i_1 * j_2;
   # VUSE <.MEM_7(D)>
   return _6;

So the IF has reversed the targets (but not the condition), and the PHIs 
in the target block have been adjusted.

Where does this happen? I cannot find it.  There doesnt seem to be 
anything in the IL which is reflecting the "expect" from the original 
code..  and if I run ranger vrp instead of classic_vrp, we don't do 
this...  so I'm missing something....

Thanks

Andrew

Re: how does vrp2 rearrange this?

[Andrew Pinski]

On Tue, Oct 19, 2021 at 1:29 PM Andrew MacLeod via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> using testcase ifcvt-4.c:
>
>
> typedef int word __attribute__((mode(word)));
>
> word
> foo (word x, word y, word a)
> {
>    word i = x;
>    word j = y;
>    /* Try to make taking the branch likely.  */
>    __builtin_expect (x > y, 1);
>    if (x > y)
>      {
>        i = a;
>        j = i;
>      }
>    return i * j;
>
> The current VRP2 pass takes:
>
>    if (x_3(D) > y_4(D))
>      goto <bb 3>; [50.00%]        <<---  note the THEN target
>    else
>      goto <bb 4>; [50.00%]
> ;;    succ:       3 [50.0% (guessed)]  count:536870912 (estimated
> locally) (TRUE_VALUE,EXECUTABLE)
> ;;                4 [50.0% (guessed)]  count:536870912 (estimated
> locally) (FALSE_VALUE,EXECUTABLE)
>
> ;;   basic block 3, loop depth 0, count 536870912 (estimated locally),
> maybe hot
> ;;    prev block 2, next block 4, flags: (NEW, REACHABLE, VISITED)
> ;;    pred:       2 [50.0% (guessed)]  count:536870912 (estimated
> locally) (TRUE_VALUE,EXECUTABLE)
> ;;    succ:       4 [always]  count:536870912 (estimated locally)
> (FALLTHRU,EXECUTABLE)
>
> ;;   basic block 4, loop depth 0, count 1073741824 (estimated locally),
> maybe hot
> ;;    prev block 3, next block 1, flags: (NEW, REACHABLE, VISITED)
> ;;    pred:       2 [50.0% (guessed)]  count:536870912 (estimated
> locally) (FALSE_VALUE,EXECUTABLE)
> ;;                3 [always]  count:536870912 (estimated locally)
> (FALLTHRU,EXECUTABLE)
>    # i_1 = PHI <x_3(D)(2), a_5(D)(3)>
>    # j_2 = PHI <y_4(D)(2), a_5(D)(3)>
>    _6 = i_1 * j_2;
>    # VUSE <.MEM_7(D)>
>    return _6;
>
> and turns it into :
>
> ;;   basic block 2, loop depth 0, count 1073741824 (estimated locally),
> maybe hot
> ;;    prev block 0, next block 3, flags: (NEW, REACHABLE, VISITED)
> ;;    pred:       ENTRY [always]  count:1073741824 (estimated locally)
> (FALLTHRU,EXECUTABLE)
>    if (x_3(D) > y_4(D))
>      goto <bb 4>; [50.00%]        <<-- has been reversed.
>    else
>      goto <bb 3>; [50.00%]
> ;;    succ:       4 [50.0% (guessed)]  count:536870912 (estimated
> locally) (TRUE_VALUE,EXECUTABLE)
> ;;                3 [50.0% (guessed)]  count:536870912 (estimated
> locally) (FALSE_VALUE,EXECUTABLE)
>
> ;;   basic block 3, loop depth 0, count 536870912 (estimated locally),
> maybe hot
> ;;    prev block 2, next block 4, flags: (NEW, VISITED)
> ;;    pred:       2 [50.0% (guessed)]  count:536870912 (estimated
> locally) (FALSE_VALUE,EXECUTABLE)
> ;;    succ:       4 [always]  count:536870912 (estimated locally)
> (FALLTHRU,EXECUTABLE)
>
> ;;   basic block 4, loop depth 0, count 1073741824 (estimated locally),
> maybe hot
> ;;    prev block 3, next block 1, flags: (NEW, REACHABLE, VISITED)
> ;;    pred:       3 [always]  count:536870912 (estimated locally)
> (FALLTHRU,EXECUTABLE)
> ;;                2 [50.0% (guessed)]  count:536870912 (estimated
> locally) (TRUE_VALUE,EXECUTABLE)
>    # i_1 = PHI <x_3(D)(3), a_5(D)(2)>
>    # j_2 = PHI <y_4(D)(3), a_5(D)(2)>
>    _6 = i_1 * j_2;
>    # VUSE <.MEM_7(D)>
>    return _6;
>
> So the IF has reversed the targets (but not the condition), and the PHIs
> in the target block have been adjusted.
>
> Where does this happen? I cannot find it.  There doesnt seem to be
> anything in the IL which is reflecting the "expect" from the original
> code..  and if I run ranger vrp instead of classic_vrp, we don't do
> this...  so I'm missing something....

I suspect this is an artifact of inserting and removing the assert
expressions in VRP rather than anything else.

Thanks,
Andrew

>
> Thanks
>
> Andrew
>

Re: how does vrp2 rearrange this?

[Andrew MacLeod]

On 10/19/21 5:13 PM, Andrew Pinski wrote:
> On Tue, Oct 19, 2021 at 1:29 PM Andrew MacLeod via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>> using testcase ifcvt-4.c:
>>
>>
>> typedef int word __attribute__((mode(word)));
>>
>> word
>> foo (word x, word y, word a)
>> {
>>     word i = x;
>>     word j = y;
>>     /* Try to make taking the branch likely.  */
>>     __builtin_expect (x > y, 1);
>>     if (x > y)
>>       {
>>         i = a;
>>         j = i;
>>       }
>>     return i * j;
>>
>> The current VRP2 pass takes:
>>
>>     if (x_3(D) > y_4(D))
>>       goto <bb 3>; [50.00%]        <<---  note the THEN target
>>     else
>>       goto <bb 4>; [50.00%]
>> ;;    succ:       3 [50.0% (guessed)]  count:536870912 (estimated
>> locally) (TRUE_VALUE,EXECUTABLE)
>> ;;                4 [50.0% (guessed)]  count:536870912 (estimated
>> locally) (FALSE_VALUE,EXECUTABLE)
>>
>> ;;   basic block 3, loop depth 0, count 536870912 (estimated locally),
>> maybe hot
>> ;;    prev block 2, next block 4, flags: (NEW, REACHABLE, VISITED)
>> ;;    pred:       2 [50.0% (guessed)]  count:536870912 (estimated
>> locally) (TRUE_VALUE,EXECUTABLE)
>> ;;    succ:       4 [always]  count:536870912 (estimated locally)
>> (FALLTHRU,EXECUTABLE)
>>
>> ;;   basic block 4, loop depth 0, count 1073741824 (estimated locally),
>> maybe hot
>> ;;    prev block 3, next block 1, flags: (NEW, REACHABLE, VISITED)
>> ;;    pred:       2 [50.0% (guessed)]  count:536870912 (estimated
>> locally) (FALSE_VALUE,EXECUTABLE)
>> ;;                3 [always]  count:536870912 (estimated locally)
>> (FALLTHRU,EXECUTABLE)
>>     # i_1 = PHI <x_3(D)(2), a_5(D)(3)>
>>     # j_2 = PHI <y_4(D)(2), a_5(D)(3)>
>>     _6 = i_1 * j_2;
>>     # VUSE <.MEM_7(D)>
>>     return _6;
>>
>> and turns it into :
>>
>> ;;   basic block 2, loop depth 0, count 1073741824 (estimated locally),
>> maybe hot
>> ;;    prev block 0, next block 3, flags: (NEW, REACHABLE, VISITED)
>> ;;    pred:       ENTRY [always]  count:1073741824 (estimated locally)
>> (FALLTHRU,EXECUTABLE)
>>     if (x_3(D) > y_4(D))
>>       goto <bb 4>; [50.00%]        <<-- has been reversed.
>>     else
>>       goto <bb 3>; [50.00%]
>> ;;    succ:       4 [50.0% (guessed)]  count:536870912 (estimated
>> locally) (TRUE_VALUE,EXECUTABLE)
>> ;;                3 [50.0% (guessed)]  count:536870912 (estimated
>> locally) (FALSE_VALUE,EXECUTABLE)
>>
>> ;;   basic block 3, loop depth 0, count 536870912 (estimated locally),
>> maybe hot
>> ;;    prev block 2, next block 4, flags: (NEW, VISITED)
>> ;;    pred:       2 [50.0% (guessed)]  count:536870912 (estimated
>> locally) (FALSE_VALUE,EXECUTABLE)
>> ;;    succ:       4 [always]  count:536870912 (estimated locally)
>> (FALLTHRU,EXECUTABLE)
>>
>> ;;   basic block 4, loop depth 0, count 1073741824 (estimated locally),
>> maybe hot
>> ;;    prev block 3, next block 1, flags: (NEW, REACHABLE, VISITED)
>> ;;    pred:       3 [always]  count:536870912 (estimated locally)
>> (FALLTHRU,EXECUTABLE)
>> ;;                2 [50.0% (guessed)]  count:536870912 (estimated
>> locally) (TRUE_VALUE,EXECUTABLE)
>>     # i_1 = PHI <x_3(D)(3), a_5(D)(2)>
>>     # j_2 = PHI <y_4(D)(3), a_5(D)(2)>
>>     _6 = i_1 * j_2;
>>     # VUSE <.MEM_7(D)>
>>     return _6;
>>
>> So the IF has reversed the targets (but not the condition), and the PHIs
>> in the target block have been adjusted.
>>
>> Where does this happen? I cannot find it.  There doesnt seem to be
>> anything in the IL which is reflecting the "expect" from the original
>> code..  and if I run ranger vrp instead of classic_vrp, we don't do
>> this...  so I'm missing something....
> I suspect this is an artifact of inserting and removing the assert
> expressions in VRP rather than anything else.
>
Well, this fails the test because  we eventually check in RTL land to 
make sure the branch was reversed due to the expect, I guess.  and it 
seems that only VRP2 does the reversing.  so its expected.. just not 
obvious to me why, when or how.

of course, the pass is th rtl.ce1 pass... and the check is for "2 true 
changes made."  . and if the IF isn't reversed , it doesnt "report" 
that.   I have no idea what thats about.

regardless... if we DONT do this, then the assembly at the end has an 
extra branch in it, so it seems like a necessary thing.

Andrew

Re: how does vrp2 rearrange this?

[Andrew Pinski]

On Tue, Oct 19, 2021 at 3:32 PM Andrew MacLeod <amacleod@redhat.com> wrote:
>
> On 10/19/21 5:13 PM, Andrew Pinski wrote:
> > On Tue, Oct 19, 2021 at 1:29 PM Andrew MacLeod via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >> using testcase ifcvt-4.c:
> >>
> >>
> >> typedef int word __attribute__((mode(word)));
> >>
> >> word
> >> foo (word x, word y, word a)
> >> {
> >>     word i = x;
> >>     word j = y;
> >>     /* Try to make taking the branch likely.  */
> >>     __builtin_expect (x > y, 1);
> >>     if (x > y)
> >>       {
> >>         i = a;
> >>         j = i;
> >>       }
> >>     return i * j;
> >>


The testcase is broken anyways.
The builtin_expect should be inside the if to have any effect.  Look
at the estimated values:
   if (x_3(D) > y_4(D))
     goto <bb 4>; [50.00%]        <<-- has been reversed.
   else
     goto <bb 3>; [50.00%]
;;    succ:       4 [50.0% (guessed)]  count:536870912 (estimated
locally) (TRUE_VALUE,EXECUTABLE)
;;                3 [50.0% (guessed)]  count:536870912 (estimated
locally) (FALSE_VALUE,EXECUTABLE)

See how it is 50/50?
The testcase is not even testing what it says it is testing.  Just
happened to work previously does not mean anything.  Move the
builtin_expect inside the if and try again. I am shocked it took this
long to find the testcase issue really.

Thanks,
Andrew Pinski


> >> The current VRP2 pass takes:
> >>
> >>     if (x_3(D) > y_4(D))
> >>       goto <bb 3>; [50.00%]        <<---  note the THEN target
> >>     else
> >>       goto <bb 4>; [50.00%]
> >> ;;    succ:       3 [50.0% (guessed)]  count:536870912 (estimated
> >> locally) (TRUE_VALUE,EXECUTABLE)
> >> ;;                4 [50.0% (guessed)]  count:536870912 (estimated
> >> locally) (FALSE_VALUE,EXECUTABLE)
> >>
> >> ;;   basic block 3, loop depth 0, count 536870912 (estimated locally),
> >> maybe hot
> >> ;;    prev block 2, next block 4, flags: (NEW, REACHABLE, VISITED)
> >> ;;    pred:       2 [50.0% (guessed)]  count:536870912 (estimated
> >> locally) (TRUE_VALUE,EXECUTABLE)
> >> ;;    succ:       4 [always]  count:536870912 (estimated locally)
> >> (FALLTHRU,EXECUTABLE)
> >>
> >> ;;   basic block 4, loop depth 0, count 1073741824 (estimated locally),
> >> maybe hot
> >> ;;    prev block 3, next block 1, flags: (NEW, REACHABLE, VISITED)
> >> ;;    pred:       2 [50.0% (guessed)]  count:536870912 (estimated
> >> locally) (FALSE_VALUE,EXECUTABLE)
> >> ;;                3 [always]  count:536870912 (estimated locally)
> >> (FALLTHRU,EXECUTABLE)
> >>     # i_1 = PHI <x_3(D)(2), a_5(D)(3)>
> >>     # j_2 = PHI <y_4(D)(2), a_5(D)(3)>
> >>     _6 = i_1 * j_2;
> >>     # VUSE <.MEM_7(D)>
> >>     return _6;
> >>
> >> and turns it into :
> >>
> >> ;;   basic block 2, loop depth 0, count 1073741824 (estimated locally),
> >> maybe hot
> >> ;;    prev block 0, next block 3, flags: (NEW, REACHABLE, VISITED)
> >> ;;    pred:       ENTRY [always]  count:1073741824 (estimated locally)
> >> (FALLTHRU,EXECUTABLE)
> >>     if (x_3(D) > y_4(D))
> >>       goto <bb 4>; [50.00%]        <<-- has been reversed.
> >>     else
> >>       goto <bb 3>; [50.00%]
> >> ;;    succ:       4 [50.0% (guessed)]  count:536870912 (estimated
> >> locally) (TRUE_VALUE,EXECUTABLE)
> >> ;;                3 [50.0% (guessed)]  count:536870912 (estimated
> >> locally) (FALSE_VALUE,EXECUTABLE)
> >>
> >> ;;   basic block 3, loop depth 0, count 536870912 (estimated locally),
> >> maybe hot
> >> ;;    prev block 2, next block 4, flags: (NEW, VISITED)
> >> ;;    pred:       2 [50.0% (guessed)]  count:536870912 (estimated
> >> locally) (FALSE_VALUE,EXECUTABLE)
> >> ;;    succ:       4 [always]  count:536870912 (estimated locally)
> >> (FALLTHRU,EXECUTABLE)
> >>
> >> ;;   basic block 4, loop depth 0, count 1073741824 (estimated locally),
> >> maybe hot
> >> ;;    prev block 3, next block 1, flags: (NEW, REACHABLE, VISITED)
> >> ;;    pred:       3 [always]  count:536870912 (estimated locally)
> >> (FALLTHRU,EXECUTABLE)
> >> ;;                2 [50.0% (guessed)]  count:536870912 (estimated
> >> locally) (TRUE_VALUE,EXECUTABLE)
> >>     # i_1 = PHI <x_3(D)(3), a_5(D)(2)>
> >>     # j_2 = PHI <y_4(D)(3), a_5(D)(2)>
> >>     _6 = i_1 * j_2;
> >>     # VUSE <.MEM_7(D)>
> >>     return _6;
> >>
> >> So the IF has reversed the targets (but not the condition), and the PHIs
> >> in the target block have been adjusted.
> >>
> >> Where does this happen? I cannot find it.  There doesnt seem to be
> >> anything in the IL which is reflecting the "expect" from the original
> >> code..  and if I run ranger vrp instead of classic_vrp, we don't do
> >> this...  so I'm missing something....
> > I suspect this is an artifact of inserting and removing the assert
> > expressions in VRP rather than anything else.
> >
> Well, this fails the test because  we eventually check in RTL land to
> make sure the branch was reversed due to the expect, I guess.  and it
> seems that only VRP2 does the reversing.  so its expected.. just not
> obvious to me why, when or how.
>
> of course, the pass is th rtl.ce1 pass... and the check is for "2 true
> changes made."  . and if the IF isn't reversed , it doesnt "report"
> that.   I have no idea what thats about.
>
> regardless... if we DONT do this, then the assembly at the end has an
> extra branch in it, so it seems like a necessary thing.
>
> Andrew
>
>
>

Re: how does vrp2 rearrange this?

[Andrew MacLeod]

On 10/19/21 7:13 PM, Andrew Pinski wrote:
> On Tue, Oct 19, 2021 at 3:32 PM Andrew MacLeod <amacleod@redhat.com> wrote:
>> On 10/19/21 5:13 PM, Andrew Pinski wrote:
>>> On Tue, Oct 19, 2021 at 1:29 PM Andrew MacLeod via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>> using testcase ifcvt-4.c:
>>>>
>>>>
>>>> typedef int word __attribute__((mode(word)));
>>>>
>>>> word
>>>> foo (word x, word y, word a)
>>>> {
>>>>      word i = x;
>>>>      word j = y;
>>>>      /* Try to make taking the branch likely.  */
>>>>      __builtin_expect (x > y, 1);
>>>>      if (x > y)
>>>>        {
>>>>          i = a;
>>>>          j = i;
>>>>        }
>>>>      return i * j;
>>>>
>
> The testcase is broken anyways.
> The builtin_expect should be inside the if to have any effect.  Look
> at the estimated values:
>     if (x_3(D) > y_4(D))
>       goto <bb 4>; [50.00%]        <<-- has been reversed.
>     else
>       goto <bb 3>; [50.00%]
> ;;    succ:       4 [50.0% (guessed)]  count:536870912 (estimated
> locally) (TRUE_VALUE,EXECUTABLE)
> ;;                3 [50.0% (guessed)]  count:536870912 (estimated
> locally) (FALSE_VALUE,EXECUTABLE)
>
> See how it is 50/50?
> The testcase is not even testing what it says it is testing.  Just
> happened to work previously does not mean anything.  Move the
> builtin_expect inside the if and try again. I am shocked it took this
> long to find the testcase issue really.
>
> Thanks,
> Andrew Pinski
>
Moving the expect around doesn't change anything, in fact, it makes it 
worse since fre and evrp immediately eliminate it as true if it is in 
the THEN block.

It looks like it is eliminated by the CDDCE pass:

cddce1 sees:

   _1 = x_5(D) > y_7(D);
   # RANGE [0, 1] NONZERO 1
   _2 = (long int) _1;
   __builtin_expect (_2, 1);
   if (x_5(D) > y_7(D))
     goto <bb 3>; [INV]
   else
     goto <bb 4>; [INV]

and proceeds:

Marking useful stmt: if (x_5(D) > y_7(D))
processing: if (x_5(D) > y_7(D))
processing: i_3 = PHI <x_5(D)(2), a_9(D)(3)>

Eliminating unnecessary statements:
Deleting : __builtin_expect (_2, 1);
Deleting : _2 = (long int) _1;
Deleting : _1 = x_5(D) > y_7(D);

IF we are suppose to reverse the If, it is not obvious to me who is 
suppose to..   You seem to be right that its a crap shot that VRP2 does 
it because there isnt enough info to dictate it.. unless somewhere it 
detects that a THEN targets an empty block which fallthrus to the ELSE 
block should be swapped.   Or maybe you are right and that it  flukeily 
happens due to the ASSERTS being added and removed.

IF i turn of DCE, then this all works like it si ssupopse to.. so maybe 
DCE isnt supopse to remove this?

Andrew

Re: how does vrp2 rearrange this?

[Andrew Pinski]

On Thu, Oct 21, 2021 at 8:04 AM Andrew MacLeod <amacleod@redhat.com> wrote:
>
> On 10/19/21 7:13 PM, Andrew Pinski wrote:
> > On Tue, Oct 19, 2021 at 3:32 PM Andrew MacLeod <amacleod@redhat.com> wrote:
> >> On 10/19/21 5:13 PM, Andrew Pinski wrote:
> >>> On Tue, Oct 19, 2021 at 1:29 PM Andrew MacLeod via Gcc-patches
> >>> <gcc-patches@gcc.gnu.org> wrote:
> >>>> using testcase ifcvt-4.c:
> >>>>
> >>>>
> >>>> typedef int word __attribute__((mode(word)));
> >>>>
> >>>> word
> >>>> foo (word x, word y, word a)
> >>>> {
> >>>>      word i = x;
> >>>>      word j = y;
> >>>>      /* Try to make taking the branch likely.  */
> >>>>      __builtin_expect (x > y, 1);
> >>>>      if (x > y)
> >>>>        {
> >>>>          i = a;
> >>>>          j = i;
> >>>>        }
> >>>>      return i * j;
> >>>>
> >
> > The testcase is broken anyways.
> > The builtin_expect should be inside the if to have any effect.  Look
> > at the estimated values:
> >     if (x_3(D) > y_4(D))
> >       goto <bb 4>; [50.00%]        <<-- has been reversed.
> >     else
> >       goto <bb 3>; [50.00%]
> > ;;    succ:       4 [50.0% (guessed)]  count:536870912 (estimated
> > locally) (TRUE_VALUE,EXECUTABLE)
> > ;;                3 [50.0% (guessed)]  count:536870912 (estimated
> > locally) (FALSE_VALUE,EXECUTABLE)
> >
> > See how it is 50/50?
> > The testcase is not even testing what it says it is testing.  Just
> > happened to work previously does not mean anything.  Move the
> > builtin_expect inside the if and try again. I am shocked it took this
> > long to find the testcase issue really.
> >
> > Thanks,
> > Andrew Pinski
> >
> Moving the expect around doesn't change anything, in fact, it makes it
> worse since fre and evrp immediately eliminate it as true if it is in
> the THEN block.

I think you misunderstood the change I was saying to do.
Try this:
typedef int word __attribute__((mode(word)));

word
foo (word x, word y, word a)
{
  word i = x;
  word j = y;
  /* Try to make taking the branch likely.  */
  if (__builtin_expect (x > y, 1))
    {
      i = a;
      j = i;
    }
  return i * j;
}
/* { dg-final { scan-rtl-dump "2 true changes made" "ce1" } } */

This should fix the "estimated values" to be more correct.

Thanks,
Andrew Pinski

>
> It looks like it is eliminated by the CDDCE pass:
>
> cddce1 sees:
>
>    _1 = x_5(D) > y_7(D);
>    # RANGE [0, 1] NONZERO 1
>    _2 = (long int) _1;
>    __builtin_expect (_2, 1);
>    if (x_5(D) > y_7(D))
>      goto <bb 3>; [INV]
>    else
>      goto <bb 4>; [INV]
>
> and proceeds:
>
> Marking useful stmt: if (x_5(D) > y_7(D))
> processing: if (x_5(D) > y_7(D))
> processing: i_3 = PHI <x_5(D)(2), a_9(D)(3)>
>
> Eliminating unnecessary statements:
> Deleting : __builtin_expect (_2, 1);
> Deleting : _2 = (long int) _1;
> Deleting : _1 = x_5(D) > y_7(D);
>
> IF we are suppose to reverse the If, it is not obvious to me who is
> suppose to..   You seem to be right that its a crap shot that VRP2 does
> it because there isnt enough info to dictate it.. unless somewhere it
> detects that a THEN targets an empty block which fallthrus to the ELSE
> block should be swapped.   Or maybe you are right and that it  flukeily
> happens due to the ASSERTS being added and removed.
>
> IF i turn of DCE, then this all works like it si ssupopse to.. so maybe
> DCE isnt supopse to remove this?
>
> Andrew
>

Re: how does vrp2 rearrange this?

[Andrew MacLeod]

[-- Attachment #1: Type: text/plain, Size: 6748 bytes --]

On 10/21/21 3:59 PM, Andrew Pinski wrote:
> On Thu, Oct 21, 2021 at 8:04 AM Andrew MacLeod <amacleod@redhat.com> wrote:
>> On 10/19/21 7:13 PM, Andrew Pinski wrote:
>>> On Tue, Oct 19, 2021 at 3:32 PM Andrew MacLeod <amacleod@redhat.com> wrote:
>>>> On 10/19/21 5:13 PM, Andrew Pinski wrote:
>>>>> On Tue, Oct 19, 2021 at 1:29 PM Andrew MacLeod via Gcc-patches
>>>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>>> using testcase ifcvt-4.c:
>>>>>>
>>>>>>
>>>>>> typedef int word __attribute__((mode(word)));
>>>>>>
>>>>>> word
>>>>>> foo (word x, word y, word a)
>>>>>> {
>>>>>>       word i = x;
>>>>>>       word j = y;
>>>>>>       /* Try to make taking the branch likely.  */
>>>>>>       __builtin_expect (x > y, 1);
>>>>>>       if (x > y)
>>>>>>         {
>>>>>>           i = a;
>>>>>>           j = i;
>>>>>>         }
>>>>>>       return i * j;
>>>>>>
>>> The testcase is broken anyways.
>>> The builtin_expect should be inside the if to have any effect.  Look
>>> at the estimated values:
>>>      if (x_3(D) > y_4(D))
>>>        goto <bb 4>; [50.00%]        <<-- has been reversed.
>>>      else
>>>        goto <bb 3>; [50.00%]
>>> ;;    succ:       4 [50.0% (guessed)]  count:536870912 (estimated
>>> locally) (TRUE_VALUE,EXECUTABLE)
>>> ;;                3 [50.0% (guessed)]  count:536870912 (estimated
>>> locally) (FALSE_VALUE,EXECUTABLE)
>>>
>>> See how it is 50/50?
>>> The testcase is not even testing what it says it is testing.  Just
>>> happened to work previously does not mean anything.  Move the
>>> builtin_expect inside the if and try again. I am shocked it took this
>>> long to find the testcase issue really.
>>>
>>> Thanks,
>>> Andrew Pinski
>>>
>> Moving the expect around doesn't change anything, in fact, it makes it
>> worse since fre and evrp immediately eliminate it as true if it is in
>> the THEN block.
> I think you misunderstood the change I was saying to do.
> Try this:
> typedef int word __attribute__((mode(word)));
>
> word
> foo (word x, word y, word a)
> {
>    word i = x;
>    word j = y;
>    /* Try to make taking the branch likely.  */
>    if (__builtin_expect (x > y, 1))
>      {
>        i = a;
>        j = i;
>      }
>    return i * j;
> }
> /* { dg-final { scan-rtl-dump "2 true changes made" "ce1" } } */
>
> This should fix the "estimated values" to be more correct.
>
> Thanks,
> Andrew Pinski

estimated values are now correct, but it changes nothing.  I think you 
are right that it could be an artifact of going in and out of ASSERT_EXPRs.

;;   basic block 2, loop depth 0, count 1073741824 (estimated locally), 
maybe hot
;;    prev block 0, next block 5, flags: (NEW, REACHABLE, VISITED)
;;    pred:       ENTRY [always]  count:1073741824 (estimated locally) 
(FALLTHRU,EXECUTABLE)
   if (x_6(D) > y_7(D))
     goto <bb 3>; [90.00%]
   else
     goto <bb 5>; [10.00%]
;;    succ:       3 [90.0% (guessed)]  count:966367640 (estimated 
locally) (TRUE_VALUE,EXECUTABLE)
;;                5 [10.0% (guessed)]  count:107374184 (estimated 
locally) (FALSE_VALUE,EXECUTABLE)

;;   basic block 5, loop depth 0, count 107374184 (estimated locally), 
maybe hot
;;    prev block 2, next block 3, flags: (NEW)
;;    pred:       2 [10.0% (guessed)]  count:107374184 (estimated 
locally) (FALSE_VALUE,EXECUTABLE)
   x_2 = ASSERT_EXPR <x_6(D), x_6(D) <= y_7(D)>;
   y_3 = ASSERT_EXPR <y_7(D), y_7(D) >= x_2>;
   goto <bb 4>; [100.00%]
;;    succ:       4 [always]  count:107374184 (estimated locally) (FALLTHRU)

;;   basic block 3, loop depth 0, count 966367640 (estimated locally), 
maybe hot
;;    prev block 5, next block 4, flags: (NEW, REACHABLE, VISITED)
;;    pred:       2 [90.0% (guessed)]  count:966367640 (estimated 
locally) (TRUE_VALUE,EXECUTABLE)
   x_1 = ASSERT_EXPR <x_6(D), x_6(D) > y_7(D)>;
   y_12 = ASSERT_EXPR <y_7(D), y_7(D) < x_1>;
;;    succ:       4 [always]  count:966367640 (estimated locally) 
(FALLTHRU,EXECUTABLE)

;;   basic block 4, loop depth 0, count 1073741824 (estimated locally), 
maybe hot
;;    prev block 3, next block 1, flags: (NEW, REACHABLE, VISITED)
;;    pred:       5 [always]  count:107374184 (estimated locally) (FALLTHRU)
;;                3 [always]  count:966367640 (estimated locally) 
(FALLTHRU,EXECUTABLE)
   # i_4 = PHI <x_2(5), a_8(D)(3)>
   # j_5 = PHI <y_3(5), a_8(D)(3)>
   _9 = i_4 * j_5;
   # VUSE <.MEM_10(D)>
   return _9;

nothing is done, and upon removing the ASSERTs, it becomes:

;;   basic block 2, loop depth 0, count 1073741824 (estimated locally), 
maybe hot
;;    prev block 0, next block 3, flags: (NEW, REACHABLE, VISITED)
;;    pred:       ENTRY [always]  count:1073741824 (estimated locally) 
(FALLTHRU,EXECUTABLE)
   if (x_6(D) > y_7(D))
     goto <bb 4>; [90.00%]
   else
     goto <bb 3>; [10.00%]
;;    succ:       4 [90.0% (guessed)]  count:966367640 (estimated 
locally) (TRUE_VALUE,EXECUTABLE)
;;                3 [10.0% (guessed)]  count:107374184 (estimated 
locally) (FALSE_VALUE,EXECUTABLE)

;;   basic block 3, loop depth 0, count 107374184 (estimated locally), 
maybe hot
;;    prev block 2, next block 4, flags: (NEW, VISITED)
;;    pred:       2 [10.0% (guessed)]  count:107374184 (estimated 
locally) (FALSE_VALUE,EXECUTABLE)
;;    succ:       4 [always]  count:107374184 (estimated locally) 
(FALLTHRU,EXECUTABLE)

;;   basic block 4, loop depth 0, count 1073741824 (estimated locally), 
maybe hot
;;    prev block 3, next block 1, flags: (NEW, REACHABLE, VISITED)
;;    pred:       3 [always]  count:107374184 (estimated locally) 
(FALLTHRU,EXECUTABLE)
;;                2 [90.0% (guessed)]  count:966367640 (estimated 
locally) (TRUE_VALUE,EXECUTABLE)
   # i_4 = PHI <x_6(D)(3), a_8(D)(2)>
   # j_5 = PHI <y_7(D)(3), a_8(D)(2)>
   _9 = i_4 * j_5;
   # VUSE <.MEM_10(D)>
   return _9;


And it appears that the testcase is checking for a situation that only 
happens because VRP2 happens to lay it out this way.

In fact, if you simply turn off VRP2 the testcase will again fail.

Even funnier/sadder...  if I make the change you suggest to the 
testcase... it then fails with current trunk... So that change actually 
disables what VRP2 happens to do :-P .

So the testcase seems problematic and fragile.

ha, even sadder, if I *reverse* what you provided and  make it:

   if (__builtin_expect (x > y, 0)),

THEN everything works as expected, and VRP becomes irrelevant. it works 
under all circumstance I can find. ( This seems to be the behaviour it 
is looking for :-P)

What do we think about reverseing that test case?  like so:

Andrew



[-- Attachment #2: K --]
[-- Type: text/plain, Size: 431 bytes --]

diff --git a/gcc/testsuite/gcc.dg/ifcvt-4.c b/gcc/testsuite/gcc.dg/ifcvt-4.c
index ec142cfd943..e74e449b402 100644
--- a/gcc/testsuite/gcc.dg/ifcvt-4.c
+++ b/gcc/testsuite/gcc.dg/ifcvt-4.c
@@ -13,8 +13,7 @@ foo (word x, word y, word a)
   word i = x;
   word j = y;
   /* Try to make taking the branch likely.  */
-  __builtin_expect (x > y, 1);
-  if (x > y)
+  if (__builtin_expect (x > y, 0))
     {
       i = a;
       j = i;