[Bug tree-optimization/101466] New: Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[Bug tree-optimization/101466] New: Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[unlvsur at live dot com]

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

            Bug ID: 101466
           Summary: Optimizers should fold bounds checking for
                    -D_GLIBCXX_ASSERTIONS=1
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: unlvsur at live dot com
  Target Milestone: ---

https://godbolt.org/z/fPdGeKMYM

Should fold together and optimize to something like this

https://godbolt.org/z/6cooP81xc

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[unlvsur at live dot com]

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

--- Comment #1 from cqwrteur <unlvsur at live dot com> ---
Created attachment 51158
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51158&action=edit
this is the assembly generated from --disable-libstdcxx-verbose

The bounds checking is still not folding together, even with this naive case.

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1 + --disable-libstdcxx-verbose

[unlvsur at live dot com]

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

--- Comment #2 from cqwrteur <unlvsur at live dot com> ---
The --enable-libstdcxx-verbose has side effects, not optimizing to
vectorization code is correct, although

.L12:
        mov     ecx, OFFSET FLAT:.LC1
        mov     edx, OFFSET FLAT:.LC2
        mov     esi, 276
        mov     edi, OFFSET FLAT:.LC3
        call    std::__replacement_assert(char const*, int, char const*, char
const*)
.L13:
        mov     ecx, OFFSET FLAT:.LC1
        mov     edx, OFFSET FLAT:.LC2
        mov     esi, 276
        mov     edi, OFFSET FLAT:.LC3
        call    std::__replacement_assert(char const*, int, char const*, char
const*)


may be merged.


However, the __builtin_abort() case can be merged together without any issue I
believe.

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1 + --disable-libstdcxx-verbose

[unlvsur at live dot com]

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

--- Comment #3 from cqwrteur <unlvsur at live dot com> ---
> 
> However, the __builtin_abort() case can be merged together without any issue
> I believe.

And optimize to vectorized code.

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-07-15
             Status|UNCONFIRMED                 |NEW
           Severity|normal                      |enhancement
           Keywords|                            |missed-optimization
            Summary|Optimizers should fold      |Optimizers should fold
                   |bounds checking for         |bounds checking for
                   |-D_GLIBCXX_ASSERTIONS=1 +   |-D_GLIBCXX_ASSERTIONS=1
                   |--disable-libstdcxx-verbose |
     Ever confirmed|0                           |1

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Reduced testcase:
extern void g() __attribute__((noreturn));

void square(int t, int *tt)
{
    if (t == 0)  g();
    tt[0] = 0;
    if (t == 1)  g();
    tt[1] = 0;
    if (t == 2)  g();
    tt[2] = 0;
    if (t == 3)   g();
    tt[3] = 0;
    if (t == 4)  g();
    tt[4] = 0;
}

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[pinskia at gcc dot gnu.org]

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

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to cqwrteur from comment #2)
> The --enable-libstdcxx-verbose has side effects, not optimizing to
> vectorization code is correct, although

Those can be merged still since the arguments to __replacement_assert are all
the same, you can change my reduced testcase to do something similar as
__replacement_assert really.  The problem is the merging of the if don't happen
if there is an statement inbetween.

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[unlvsur at live dot com]

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

--- Comment #6 from cqwrteur <unlvsur at live dot com> ---
(In reply to Andrew Pinski from comment #4)
> Reduced testcase:
> extern void g() __attribute__((noreturn));
> 
> void square(int t, int *tt)
> {
>     if (t == 0)  g();
>     tt[0] = 0;
>     if (t == 1)  g();
>     tt[1] = 0;
>     if (t == 2)  g();
>     tt[2] = 0;
>     if (t == 3)   g();
>     tt[3] = 0;
>     if (t == 4)  g();
>     tt[4] = 0;
> }

I think the reduced testcase should be

extern void g() __attribute__((noreturn));

void square(unsigned t, int *tt)
{
    if (t<=0)  g();
    tt[0] = 0;
    if (t<=1)  g();
    tt[1] = 0;
    if (t<=2)  g();
    tt[2] = 0;
    if (t<=3)   g();
    tt[3] = 0;
    if (t<=4)  g();
    tt[4] = 0;
}

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[pinskia at gcc dot gnu.org]

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

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to cqwrteur from comment #6)
> (In reply to Andrew Pinski from comment #4)
> > Reduced testcase:
> > extern void g() __attribute__((noreturn));
> > 
> > void square(int t, int *tt)
> > {
> >     if (t == 0)  g();
> >     tt[0] = 0;
> >     if (t == 1)  g();
> >     tt[1] = 0;
> >     if (t == 2)  g();
> >     tt[2] = 0;
> >     if (t == 3)   g();
> >     tt[3] = 0;
> >     if (t == 4)  g();
> >     tt[4] = 0;
> > }
> 
> I think the reduced testcase should be
> 
> extern void g() __attribute__((noreturn));
> 
> void square(unsigned t, int *tt)
> {
>     if (t<=0)  g();
>     tt[0] = 0;
>     if (t<=1)  g();
>     tt[1] = 0;
>     if (t<=2)  g();
>     tt[2] = 0;
>     if (t<=3)   g();
>     tt[3] = 0;
>     if (t<=4)  g();
>     tt[4] = 0;
> }

Yes obvious t should be unsigned.  But the <= will become == due to VRP
figuring that out :).  So it is either way.

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
I don't see what we can optimize for the reduced testcases.

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[unlvsur at live dot com]

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

--- Comment #9 from cqwrteur <unlvsur at live dot com> ---
(In reply to Richard Biener from comment #8)
> I don't see what we can optimize for the reduced testcases.

void square(unsigned t, int *tt)
{
    if (t<=0)  __builtin_abort();
    tt[0] = 0;
    if (t<=1)  __builtin_abort();
    tt[1] = 0;
    if (t<=2)  __builtin_abort();
    tt[2] = 0;
    if (t<=3)   __builtin_abort();
    tt[3] = 0;
    if (t<=4)  __builtin_abort();
    tt[4] = 0;
}

but this should be able to optimize.

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[rguenth at gcc dot gnu.org]

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

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to cqwrteur from comment #9)
> (In reply to Richard Biener from comment #8)
> > I don't see what we can optimize for the reduced testcases.
> 
> void square(unsigned t, int *tt)
> {
>     if (t<=0)  __builtin_abort();
>     tt[0] = 0;
>     if (t<=1)  __builtin_abort();
>     tt[1] = 0;
>     if (t<=2)  __builtin_abort();
>     tt[2] = 0;
>     if (t<=3)   __builtin_abort();
>     tt[3] = 0;
>     if (t<=4)  __builtin_abort();
>     tt[4] = 0;
> }
> 
> but this should be able to optimize.

How?  Can you show the desired result?

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[rguenth at gcc dot gnu.org]

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

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
With some hand-waving we could generate

void square(unsigned t, int *tt)
{
    if (t<=4)  __builtin_abort();
    tt[0] = 0;
    tt[1] = 0;
    tt[2] = 0;
    tt[3] = 0;
    tt[4] = 0;
}

but I don't see how it fits any existing transform?  The "hand-waving"
would be that __builtin_abort () since it's a known function cannot
observe the dropped side-effects like tt[0] = 0 when t > 0.

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[rguenth at gcc dot gnu.org]

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

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #11)
> With some hand-waving we could generate
> 
> void square(unsigned t, int *tt)
> {
>     if (t<=4)  __builtin_abort();
>     tt[0] = 0;
>     tt[1] = 0;
>     tt[2] = 0;
>     tt[3] = 0;
>     tt[4] = 0;
> }
> 
> but I don't see how it fits any existing transform?  The "hand-waving"
> would be that __builtin_abort () since it's a known function cannot
> observe the dropped side-effects like tt[0] = 0 when t > 0.

That is, we'd sink the stores across the abort ()s because they are
not uses of them, then we arrive at

    if (t<=0)  __builtin_abort();
    if (t<=1)  __builtin_abort();
    if (t<=2)  __builtin_abort();
    if (t<=3)  __builtin_abort();
    if (t<=4)  __builtin_abort();
    tt[0] = 0;
    tt[1] = 0;
    tt[2] = 0;
    tt[3] = 0;
    tt[4] = 0;

where we'd somehow "thread" to a single condition (PRE tail merging
merges the abort () blocks and reassoc way after it manages to merge
the controlling conditions).

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[unlvsur at live dot com]

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

--- Comment #13 from cqwrteur <unlvsur at live dot com> ---
(In reply to Richard Biener from comment #12)
> (In reply to Richard Biener from comment #11)
> > With some hand-waving we could generate
> > 
> > void square(unsigned t, int *tt)
> > {
> >     if (t<=4)  __builtin_abort();
> >     tt[0] = 0;
> >     tt[1] = 0;
> >     tt[2] = 0;
> >     tt[3] = 0;
> >     tt[4] = 0;
> > }
> > 
> > but I don't see how it fits any existing transform?  The "hand-waving"
> > would be that __builtin_abort () since it's a known function cannot
> > observe the dropped side-effects like tt[0] = 0 when t > 0.
> 
> That is, we'd sink the stores across the abort ()s because they are
> not uses of them, then we arrive at
> 
>     if (t<=0)  __builtin_abort();
>     if (t<=1)  __builtin_abort();
>     if (t<=2)  __builtin_abort();
>     if (t<=3)  __builtin_abort();
>     if (t<=4)  __builtin_abort();
>     tt[0] = 0;
>     tt[1] = 0;
>     tt[2] = 0;
>     tt[3] = 0;
>     tt[4] = 0;
> 
> where we'd somehow "thread" to a single condition (PRE tail merging
> merges the abort () blocks and reassoc way after it manages to merge
> the controlling conditions).

https://godbolt.org/z/af8nn8K4h

I found this to be interesting. with -fno-exceptions, all those calls are
folded together. It looks -fno-exceptions hurts optimizations here. Is that a
bug or C++ EH truly affects the semantics here?

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[rguenther at suse dot de]

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

--- Comment #14 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 19 Jul 2021, unlvsur at live dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101466
> 
> --- Comment #13 from cqwrteur <unlvsur at live dot com> ---
> (In reply to Richard Biener from comment #12)
> > (In reply to Richard Biener from comment #11)
> > > With some hand-waving we could generate
> > > 
> > > void square(unsigned t, int *tt)
> > > {
> > >     if (t<=4)  __builtin_abort();
> > >     tt[0] = 0;
> > >     tt[1] = 0;
> > >     tt[2] = 0;
> > >     tt[3] = 0;
> > >     tt[4] = 0;
> > > }
> > > 
> > > but I don't see how it fits any existing transform?  The "hand-waving"
> > > would be that __builtin_abort () since it's a known function cannot
> > > observe the dropped side-effects like tt[0] = 0 when t > 0.
> > 
> > That is, we'd sink the stores across the abort ()s because they are
> > not uses of them, then we arrive at
> > 
> >     if (t<=0)  __builtin_abort();
> >     if (t<=1)  __builtin_abort();
> >     if (t<=2)  __builtin_abort();
> >     if (t<=3)  __builtin_abort();
> >     if (t<=4)  __builtin_abort();
> >     tt[0] = 0;
> >     tt[1] = 0;
> >     tt[2] = 0;
> >     tt[3] = 0;
> >     tt[4] = 0;
> > 
> > where we'd somehow "thread" to a single condition (PRE tail merging
> > merges the abort () blocks and reassoc way after it manages to merge
> > the controlling conditions).
> 
> https://godbolt.org/z/af8nn8K4h
> 
> I found this to be interesting. with -fno-exceptions, all those calls are
> folded together. It looks -fno-exceptions hurts optimizations here. Is that a
> bug or C++ EH truly affects the semantics here?

It's likely not "semantics" but the pass responsible likey simply gives
up on incoming EH edges.  There's some long-term TODO to base that
off some more modern tooling.

[Bug tree-optimization/101466] Optimizers should fold bounds checking for -D_GLIBCXX_ASSERTIONS=1

[unlvsur at live dot com]

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

--- Comment #15 from cqwrteur <unlvsur at live dot com> ---
(In reply to rguenther@suse.de from comment #14)
> On Mon, 19 Jul 2021, unlvsur at live dot com wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101466
> > 
> > --- Comment #13 from cqwrteur <unlvsur at live dot com> ---
> > (In reply to Richard Biener from comment #12)
> > > (In reply to Richard Biener from comment #11)
> > > > With some hand-waving we could generate
> > > > 
> > > > void square(unsigned t, int *tt)
> > > > {
> > > >     if (t<=4)  __builtin_abort();
> > > >     tt[0] = 0;
> > > >     tt[1] = 0;
> > > >     tt[2] = 0;
> > > >     tt[3] = 0;
> > > >     tt[4] = 0;
> > > > }
> > > > 
> > > > but I don't see how it fits any existing transform?  The "hand-waving"
> > > > would be that __builtin_abort () since it's a known function cannot
> > > > observe the dropped side-effects like tt[0] = 0 when t > 0.
> > > 
> > > That is, we'd sink the stores across the abort ()s because they are
> > > not uses of them, then we arrive at
> > > 
> > >     if (t<=0)  __builtin_abort();
> > >     if (t<=1)  __builtin_abort();
> > >     if (t<=2)  __builtin_abort();
> > >     if (t<=3)  __builtin_abort();
> > >     if (t<=4)  __builtin_abort();
> > >     tt[0] = 0;
> > >     tt[1] = 0;
> > >     tt[2] = 0;
> > >     tt[3] = 0;
> > >     tt[4] = 0;
> > > 
> > > where we'd somehow "thread" to a single condition (PRE tail merging
> > > merges the abort () blocks and reassoc way after it manages to merge
> > > the controlling conditions).
> > 
> > https://godbolt.org/z/af8nn8K4h
> > 
> > I found this to be interesting. with -fno-exceptions, all those calls are
> > folded together. It looks -fno-exceptions hurts optimizations here. Is that a
> > bug or C++ EH truly affects the semantics here?
> 
> It's likely not "semantics" but the pass responsible likey simply gives
> up on incoming EH edges.  There's some long-term TODO to base that
> off some more modern tooling.

I do not see anything wrong to assume the semantics of __builtin_trap() and
__builtin_abort() for compiler. Just like we treat memcpy or __builtin_memcpy
or memmove as some kind of magic intrinsic in the compiler. Even memcpy maybe
implemented by libc differently.

So this optimization is technically valid.