* how does vrp2 rearrange this?
@ 2021-10-19 20:28 Andrew MacLeod
2021-10-19 21:13 ` Andrew Pinski
0 siblings, 1 reply; 7+ messages in thread
From: Andrew MacLeod @ 2021-10-19 20:28 UTC (permalink / raw)
To: gcc-patches, Richard Biener, Jeff Law, Aldy Hernandez
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: how does vrp2 rearrange this? 2021-10-19 20:28 how does vrp2 rearrange this? Andrew MacLeod @ 2021-10-19 21:13 ` Andrew Pinski 2021-10-19 22:32 ` Andrew MacLeod 0 siblings, 1 reply; 7+ messages in thread From: Andrew Pinski @ 2021-10-19 21:13 UTC (permalink / raw) To: Andrew MacLeod; +Cc: gcc-patches, Richard Biener, Jeff Law, Aldy Hernandez 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: how does vrp2 rearrange this? 2021-10-19 21:13 ` Andrew Pinski @ 2021-10-19 22:32 ` Andrew MacLeod 2021-10-19 23:13 ` Andrew Pinski 0 siblings, 1 reply; 7+ messages in thread From: Andrew MacLeod @ 2021-10-19 22:32 UTC (permalink / raw) To: Andrew Pinski; +Cc: gcc-patches, Richard Biener, Jeff Law, Aldy Hernandez 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: how does vrp2 rearrange this? 2021-10-19 22:32 ` Andrew MacLeod @ 2021-10-19 23:13 ` Andrew Pinski 2021-10-21 15:03 ` Andrew MacLeod 0 siblings, 1 reply; 7+ messages in thread From: Andrew Pinski @ 2021-10-19 23:13 UTC (permalink / raw) To: Andrew MacLeod; +Cc: gcc-patches, Richard Biener, Jeff Law, Aldy Hernandez 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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: how does vrp2 rearrange this? 2021-10-19 23:13 ` Andrew Pinski @ 2021-10-21 15:03 ` Andrew MacLeod 2021-10-21 19:59 ` Andrew Pinski 0 siblings, 1 reply; 7+ messages in thread From: Andrew MacLeod @ 2021-10-21 15:03 UTC (permalink / raw) To: Andrew Pinski; +Cc: gcc-patches, Richard Biener, Jeff Law, Aldy Hernandez 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: how does vrp2 rearrange this? 2021-10-21 15:03 ` Andrew MacLeod @ 2021-10-21 19:59 ` Andrew Pinski 2021-10-25 15:43 ` Andrew MacLeod 0 siblings, 1 reply; 7+ messages in thread From: Andrew Pinski @ 2021-10-21 19:59 UTC (permalink / raw) To: Andrew MacLeod; +Cc: gcc-patches, Richard Biener, Jeff Law, Aldy Hernandez 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: how does vrp2 rearrange this? 2021-10-21 19:59 ` Andrew Pinski @ 2021-10-25 15:43 ` Andrew MacLeod 0 siblings, 0 replies; 7+ messages in thread From: Andrew MacLeod @ 2021-10-25 15:43 UTC (permalink / raw) To: Andrew Pinski; +Cc: gcc-patches, Richard Biener, Jeff Law, Aldy Hernandez [-- 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-25 15:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-19 20:28 how does vrp2 rearrange this? Andrew MacLeod 2021-10-19 21:13 ` Andrew Pinski 2021-10-19 22:32 ` Andrew MacLeod 2021-10-19 23:13 ` Andrew Pinski 2021-10-21 15:03 ` Andrew MacLeod 2021-10-21 19:59 ` Andrew Pinski 2021-10-25 15:43 ` Andrew MacLeod
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).