[Bug rtl-optimization/108992] New: Regression: Branch direction canonicalization leads to pointless tail duplication / CSE/sinking by inverting branch

[Bug rtl-optimization/108992] New: Regression: Branch direction canonicalization leads to pointless tail duplication / CSE/sinking by inverting branch

[nok.raven at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108992

            Bug ID: 108992
           Summary: Regression: Branch direction canonicalization leads to
                    pointless tail duplication / CSE/sinking by inverting
                    branch
           Product: gcc
           Version: 6.0
            Status: UNCONFIRMED
          Keywords: missed-optimization
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: nok.raven at gmail dot com
  Target Milestone: ---

There are two regressions, in GCC 7 and in GCC 8. If you invert branch manually
(replace 'cond' with '!cond') - the results were the same previously.

// Regressed since GCC 7: https://godbolt.org/z/h4brz7zG9
void use(int *);
void use2(int *);

void foo(bool cond, int * p)
{
    if (cond) {
        use(p);
    }
    use2(p);
}

// GCC 6
foo(bool, int*):
        test    dil, dil
        push    rbx
        mov     rbx, rsi
        je      .L2
        mov     rdi, rsi
        call    use(int*)
.L2:
        mov     rdi, rbx
        pop     rbx
        jmp     use2(int*)

// GCC 7
foo(bool, int*):
        test    dil, dil
        jne     .L8
        mov     rdi, rsi
        jmp     use2(int*)
.L8:
        sub     rsp, 24
        mov     rdi, rsi
        mov     QWORD PTR [rsp+8], rsi
        call    use2(int*)
        mov     rsi, QWORD PTR [rsp+8]
        add     rsp, 24
        mov     rdi, rsi
        jmp     use2(int*)


// Regressed since GCC 8: https://godbolt.org/z/MjxqTnbKa
void use(int *);
void use2(int *);

void foo(int * p, bool cond)
{
    if (cond) {
        use(p);
    }
    use2(p);
}

// GCC 7
foo(int*, bool):
        test    sil, sil
        push    rbx
        mov     rbx, rdi
        je      .L2
        call    use(int*)
.L2:
        mov     rdi, rbx
        pop     rbx
        jmp     use2(int*)

// GCC 8
foo(int*, bool):
        push    rbx
        mov     rbx, rdi
        test    sil, sil
        jne     .L5
        mov     rdi, rbx
        pop     rbx
        jmp     use2(int*)
.L5:
        call    use(int*)
        mov     rdi, rbx
        pop     rbx
        jmp     use2(int*)

[Bug rtl-optimization/108992] Regression: Branch direction canonicalization leads to pointless tail duplication / CSE/sinking by inverting branch

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108992

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Why do you think this is a bug?
I don't see anything wrong with the newer versions of gcc.
Duplicating the basic blocks is done on purpose for speed reasons.

[Bug rtl-optimization/108992] Regression: Branch direction canonicalization leads to pointless tail duplication / CSE/sinking by inverting branch

[nok.raven at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108992

--- Comment #2 from Nikita Kniazev <nok.raven at gmail dot com> ---
> Why do you think this is a bug?
> I don't see anything wrong with the newer versions of gcc.
> Duplicating the basic blocks is done on purpose for speed reasons.

I understand that removing diamonds is done to open more optimization
opportunities but in this case there is none; not undoing the cloning leads to
code size increases which creates unnecessary pressure on instruction cache.

[Bug rtl-optimization/108992] Regression: Branch direction canonicalization leads to pointless tail duplication / CSE/sinking by inverting branch

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108992

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Did you see this in real code or you just noticed this while looking at code
generation?

[Bug middle-end/108992] Regression: Branch direction canonicalization leads to pointless tail duplication / CSE/sinking by inverting branch

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108992

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I suspect this is not a bug, GCC tries to optimze the fast path into straight
line code without any waste of space.
In the first case GCC predicts that the cond is going to be true 66% of the
time because there is comparison against 0 prediction going in the heurstics.

For the first testcase if you do:
void use(int *);
void use2(int *);

void foo(bool cond, int * p)
{
    if (__builtin_expect(cond, 1)) {
        use(p);
    }
    use2(p);
}

Then you get the result you want.
Adding the builtin_expect for the second case you get the same too.
Basically GCC is pushing what it thinks as cold code away from the original
path.

[Bug middle-end/108992] Regression: Branch direction canonicalization leads to pointless tail duplication / CSE/sinking by inverting branch

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108992

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Note I filed PR 108997 for what seems like the wrong heurstic for the
prediction.

[Bug middle-end/108992] Regression: Branch direction canonicalization leads to pointless tail duplication / CSE/sinking by inverting branch

[nok.raven at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108992

--- Comment #6 from Nikita Kniazev <nok.raven at gmail dot com> ---
> Did you see this in real code or you just noticed this while looking at code generation?

If you mean do I have any benchmark - unfortunately no. I noticed it for a
while by poking different code to compare Clang codegen to GCC.

> In the first case GCC predicts that the cond is going to be true 66% of the time

The 66% thing is what I also noticed for a while.

> because there is comparison against 0 prediction going in the heurstics.

The duplication happens even if I make cond int and compare with any other
value

void use(int *);
void use2(int *);

void foo(int * p, int cond)
{
    if (cond == 789) {
        use(p);
    }
    use2(p);
}

[Bug middle-end/108992] Regression: Branch direction canonicalization leads to pointless tail duplication / CSE/sinking by inverting branch

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108992

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Nikita Kniazev from comment #6)
> The duplication happens even if I make cond int and compare with any other
> value
> 
> void use(int *);
> void use2(int *);
> 
> void foo(int * p, int cond)
> {
>     if (cond == 789) {
>         use(p);
>     }
>     use2(p);
> }

Yes and no, Yes it does in GCC 7/8 but starting in GCC 9 there is not.
GCC before GCC 10.x is no longer supported so there is not much we can do
there.

[Bug middle-end/108992] Regression: Branch direction canonicalization leads to pointless tail duplication / CSE/sinking by inverting branch

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108992

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #7)
> (In reply to Nikita Kniazev from comment #6)
> > The duplication happens even if I make cond int and compare with any other
> > value
> > 
> > void use(int *);
> > void use2(int *);
> > 
> > void foo(int * p, int cond)
> > {
> >     if (cond == 789) {
> >         use(p);
> >     }
> >     use2(p);
> > }
> 
> Yes and no, Yes it does in GCC 7/8 but starting in GCC 9 there is not.
> GCC before GCC 10.x is no longer supported so there is not much we can do
> there.

Well actually I was wrong, the duplication is there but you can see why GCC
does the code duplication really. To also do shrink wrapping.
That is GCC 9+ does not need to setup a stack frame for the != 789 case and
just tail call to use2.