On Tue, Oct 19, 2021 at 3:32 PM Andrew MacLeod <amacl...@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
>
>
>
