[Bug c++/94485] New: [10.0.1, c++2a] g++ optimizes away the code, accepts arbitrary inline asm

[Bug c++/94485] New: [10.0.1, c++2a] g++ optimizes away the code, accepts arbitrary inline asm

[dimitri.gorokhovik at free dot fr]

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

            Bug ID: 94485
           Summary: [10.0.1, c++2a] g++ optimizes away the code, accepts
                    arbitrary inline asm
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dimitri.gorokhovik at free dot fr
  Target Milestone: ---

Created attachment 48194
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48194&action=edit
Sample code illustrating the sighting.

Version: gcc (GCC) 10.0.1 20200404 (experimental)

command line:
~/gcc-trunk/dist/bin/g++ -std=c++2a  -O3 -Wall -Wextra -W bug-202004041709.cpp
-o bug-1 && ./bug-1


The attached file (apologies, still big) contains the following code (line
425):

  constexpr iterator (tesselation const& me)
   : cube_ { me.cube_ },
     inner_ { me.as_parent ().begin () },
     tess_ {},
     outer_ { tess_ }
  {
   if (inner_)
   {
     tess_ = tesselation_of_two_cubes { cube_, *inner_ };
     asm("before:");
#if 0 // breaks only with -O3
     auto const tmp { tess_.begin () };
     asm("middle:");
     outer_ = tmp;
     ^^^^^^^^^^^^
#else // breaks with -O3, -O1, fixes with -fsanitize=undefined|null
     outer_ = { tess_.begin () };
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     asm("after:");
#endif
     inner_.next ();
     if (! outer_) throw 749;
   };
  };

(Minor remark: 'asm' statements in this constexpr method raise no brows. I
tried putting there insns that require evaluation, seems anything is silently
accepted. The same asm statements are rejected in a more simple constexpr
code.)

It seems, some optimization levels completely remove the highlighted assignment
so that 'throw 749' is executed (main() prints it then). Other passes keep the
copying code and produce expected result. This over-optimization can be fixed
by enabling certain sanitizers: undefined, null, vptr.

The asm code produced:

-- broken (compiled with -O3 or -O1):
#APP
# 425 "bug-1.cpp" 1
        before:
# 0 "" 2
#NO_APP
        movq    %r12, 216(%rsp)
#APP
# 433 "bug-1.cpp" 1
        after:
# 0 "" 2
#NO_APP

-- working (compiled with -O0 or -O1 or with -fsanitizer=null):
#APP
# 425 "bug-1.cpp" 1
        before:
# 0 "" 2
#NO_APP
        xorl    %eax, %eax
        cmpb    $0, 43(%rbx)
        movq    %r14, 64(%rsp)
        movw    %ax, 72(%rsp)
        movb    $0, 74(%rsp)
        je      .L73
        cmpb    $0, 63(%rbx)
        je      .L73
        cmpb    $0, 83(%rbx)
        sete    %al
.L68:
        movb    %al, 75(%rsp)
        movq    64(%rsp), %rax
        movdqa  64(%rsp), %xmm6
        movq    %rax, 88(%rbx)
        movl    72(%rsp), %eax
        movaps  %xmm6, 96(%rsp)
        movl    %eax, 96(%rbx)
#APP
# 433 "bug-1.cpp" 1
        after:
# 0 "" 2
#NO_APP

[Bug c++/94485] [10.0.1, c++2a] g++ optimizes away necessary code, accepts arbitrary inline asm

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Without looking fully into this, it seems to me a variable is being used after
it goes out of scope.

[Bug c++/94485] [10.0.1, c++2a] g++ optimizes away necessary code, accepts arbitrary inline asm

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Also c++20 changes constexpr is handled which is why inline asm is accepted now
IIRC.

[Bug c++/94485] [10.0.1, c++2a] g++ optimizes away necessary code, accepts arbitrary inline asm

[dimitri.gorokhovik at free dot fr]

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

--- Comment #3 from Dimitri Gorokhovik <dimitri.gorokhovik at free dot fr> ---
(In reply to Andrew Pinski from comment #2)
> Also c++20 changes constexpr is handled which is why inline asm is accepted
> now IIRC.

This code:

constexpr auto f()
{
  asm("before:");
  constexpr auto r = 129;
  asm("after:");

  return r;
};

produces:
bug-2.cpp: In function ‘constexpr auto f()’:
bug-2.cpp:3:3: error: inline assembly is not a constant expression
    3 |   asm("before:");
      |   ^~~
bug-2.cpp:3:3: note: only unevaluated inline assembly is allowed in a
‘constexpr’ function in C++2a

[Bug c++/94485] [10.0.1, c++2a] g++ optimizes away necessary code, accepts arbitrary inline asm

[dimitri.gorokhovik at free dot fr]

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

--- Comment #4 from Dimitri Gorokhovik <dimitri.gorokhovik at free dot fr> ---
To be clear, the braces here:

     outer_ = { tess_.begin () };

are the leftover of the simplification. Plain assignment also gets removed,
only it is slightly harder to reproduce (-O3 only).

[Bug c++/94485] [10.0.1, c++2a] g++ optimizes away necessary code, accepts arbitrary inline asm

[dimitri.gorokhovik at free dot fr]

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

--- Comment #5 from Dimitri Gorokhovik <dimitri.gorokhovik at free dot fr> ---
Build "g++ (GCC) 10.0.1 20200418 (experimental)" does not reproduce the issue
on the attached sample code anymore. 

However:

-- adding an asm statement into the sample code gets the issue back. Tried asm
("nop") and asm ("") on line 426, other insns seem to be vetted and rejected.

-- issue is still reproduced on the original code where it is not related to
the asm, but rather to use of a pointer as opposed to a copy. 

-- vetting of asm content doesn't seem to be performed in the original code.

[Bug c++/94485] [10.0.1, c++2a] g++ optimizes away necessary code, accepts arbitrary inline asm

[dimitri.gorokhovik at free dot fr]

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

--- Comment #6 from Dimitri Gorokhovik <dimitri.gorokhovik at free dot fr> ---
I bisected this issue to the commit:

commit 35a952ba70028b50fbd2fd19f7dc91f2da45371e
Author: Martin Jambor <mjambor@suse.cz>
Date:   Tue Jun 13 13:34:51 2017 +0200

    [PR80803 1/2] Streamline SRA access enqueuing

    2017-06-13  Martin Jambor  <mjambor@suse.cz>

            * tree-sra.c (add_access_to_work_queue): Only enqueue accesses
            that have a first_link.
            (sort_and_splice_var_accesses): Do not check first_link before
            enquing.
            (subtree_mark_written_and_enqueue): Likewise.
            (propagate_all_subaccesses): Likewise and do not stop at first
            parent with a first_link.

    From-SVN: r249153

on the branch gcc-8 and on. (gcc-7.5.0 and clang-11 don't have the issue.
Adding 2/2 of the change doesn't help.)

So I have the following fragment of code:

template <unsigned N>
struct index : public std::array <unsigned char, N>
{
         constexpr index () : std::array <unsigned char, N> {0} {};
         constexpr bool can_be_incremented (tesselation_of_two_cubes const&);
};
...

struct iterator
{
        tesselation_of_two_cubes const* me_;
        index <number_of_dimensions> idx_;
        bool reached_end_;

        constexpr iterator (tesselation_of_two_cubes const& ts) noexcept
                : me_ { &ts },
                  idx_ {},                        
        >>>>>>>> reached_end_ { ! idx_.can_be_incremented (ts) } <<<<<<<<<<
eliminated
        {};
...

If compiling as shown, SRA and subsequent passes make the line above go away
(replace by 'true' which shouldn't be). This is because the ctor for idx_ is
also eliminated, so idx_.can_be_incremented() probably considered reading from
uninitialised data by SRA.

If I change the layout of 'struct iterator', move the member 'idx_' above
'me_', or below 'reached_end_', the issue goes away.

Still struggling with producing small good code sample.

[Bug c++/94485] [10.0.1, c++2a] g++ optimizes away necessary code, accepts arbitrary inline asm

[dimitri.gorokhovik at free dot fr]

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

Dimitri Gorokhovik <dimitri.gorokhovik at free dot fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #48194|0                           |1
        is obsolete|                            |

--- Comment #7 from Dimitri Gorokhovik <dimitri.gorokhovik at free dot fr> ---
Created attachment 49268
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49268&action=edit
Testcase demonstrating the issue.

A more reduced testcase demonstrating the issue.

[Bug c++/94485] Inter-dependency between { tree-sra, ABI, inlining, loop-unrolling } leads to mis-optimization

[dimitri.gorokhovik at free dot fr]

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

--- Comment #8 from Dimitri Gorokhovik <dimitri.gorokhovik at free dot fr> ---
I was able to reduce same code (see the attached file bug-6.cpp).

-- when compiled correctly, running it produces the following (expected)
output:

cube: ({ 0, 0, 0 }, { 1, 1, 1 }) 
cube: ({ 0, 0, 1 }, { 1, 1, 2 }) 
cube: ({ 0, 0, 2 }, { 1, 1, 3 }) 
cube: ({ 0, 1, 0 }, { 1, 2, 1 }) 
cube: ({ 0, 1, 1 }, { 1, 2, 2 }) 
cube: ({ 0, 1, 2 }, { 1, 2, 3 }) 
cube: ({ 0, 2, 0 }, { 1, 3, 1 }) 
cube: ({ 0, 2, 1 }, { 1, 3, 2 }) 
cube: ({ 0, 2, 2 }, { 1, 3, 3 }) 
cube: ({ 1, 0, 0 }, { 2, 1, 1 }) 
cube: ({ 1, 0, 1 }, { 2, 1, 2 }) 
cube: ({ 1, 0, 2 }, { 2, 1, 3 }) 
cube: ({ 1, 1, 0 }, { 2, 2, 1 }) 
cube: ({ 1, 1, 1 }, { 2, 2, 2 }) 
cube: ({ 1, 1, 2 }, { 2, 2, 3 }) 
cube: ({ 1, 2, 0 }, { 2, 3, 1 }) 
cube: ({ 1, 2, 1 }, { 2, 3, 2 }) 
cube: ({ 1, 2, 2 }, { 2, 3, 3 }) 
cube: ({ 2, 0, 0 }, { 3, 1, 1 }) 
cube: ({ 2, 0, 1 }, { 3, 1, 2 }) 
cube: ({ 2, 0, 2 }, { 3, 1, 3 }) 
cube: ({ 2, 1, 0 }, { 3, 2, 1 }) 
cube: ({ 2, 1, 1 }, { 3, 2, 2 }) 
cube: ({ 2, 1, 2 }, { 3, 2, 3 }) 
cube: ({ 2, 2, 0 }, { 3, 3, 1 }) 
cube: ({ 2, 2, 1 }, { 3, 3, 2 }) 
cube: ({ 2, 2, 2 }, { 3, 3, 3 }) 
count = 27

-- when compiled incorrectly, it prints out:

count = 0

Tested with build g++ (GCC) 11.0.0 20200924 (experimental).


In order to compile and run:

g++ -std=c++17 -O3 -o bug-6 bug-6.cpp && ./bug-6

This builds for implicit '-m64' (x86_64) and produces invalid output. 

To get valid output, compile with either of the following:
-m32
-O0 (instead of -O3)
-fno-tree-sra
one of: -DFIX_0, -DFIX_1, -DFIX_2, -DFIX_3, -DFIX_4 


>From my limited understanding of tree dumps, here is what roughly happens:

-- the routine 'begin()', line 183, returns 'struct iterator' by value. The
latter has the size of 14 bytes so returned "in registers". Forcing it to be
returned via memory ==> issue goes away. (Methods to force: make bigger than 16
bytes, make some fields volatile, use -m32). Note also that, when the routine
is evaluated as constexpr (in static_assert), the issue is not reproduced.

-- all called routines (pretty much) are inlined inside one call, to
'count_them ()'. Prevent the inlining of the routine 'can_be_incremented ()'
==>  issue goes away. (Methods to prevent: define FIX_1.)

-- SRA replaces several fields of the 'struct iterator' (line 150), most
importantly 'idx_' (line 153). Disable SRA ==> issue goes away (-fno-tree-sra
or  -O0). 

This replacement by tree-SRA somehow doesn't propagate the writes to the
replacement vars of idx_to the original parts of the structure living "in the
return registers".  When the return value lives in memory, the writes are
propagated correctly.

The compiler then eliminates the loop in 'can_be_incremented' and evaluates the
call to that routine to 'false' (line 163). Forcibly keeping the loop (-DFIX_2)
or replacing it by non-loop code (-DFIX_0) ==> issue goes away.

[Bug ipa/94485] Inter-dependency between { tree-sra, ABI, inlining, loop-unrolling } leads to mis-optimization

[sjames at gcc dot gnu.org]

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

Sam James <sjames at gcc dot gnu.org> changed:

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

--- Comment #9 from Sam James <sjames at gcc dot gnu.org> ---
Not sure if I'm doing something wrong or if it just got fixed in the 10 dev
cycle, but https://godbolt.org/z/x48xK9j67 is fine and so were the other
compilers I tried (9+).

[Bug ipa/94485] Inter-dependency between { tree-sra, ABI, inlining, loop-unrolling } leads to mis-optimization

[sjames at gcc dot gnu.org]

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

--- Comment #10 from Sam James <sjames at gcc dot gnu.org> ---
ok, on godbolt, 8.5 fails. I think we're fine here then?

[Bug ipa/94485] Inter-dependency between { tree-sra, ABI, inlining, loop-unrolling } leads to mis-optimization

[pinskia at gcc dot gnu.org]

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

--- Comment #11 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
SRA in 10.2.0:
```
Created a replacement for D.14474 offset: 0, size: 64: SR.70D.14479
Created a replacement for D.14475 offset: 0, size: 64: SR.71D.14480
Removing load: D.13665 = D.14475;
```

in 10.3.0:
```
Created a replacement for D.14507 offset: 0, size: 64: SR.70D.14512
Created a replacement for D.14508 offset: 0, size: 64: SR.71D.14513
```

Notice how there is not a "removing load" in 10.3.0.

[Bug ipa/94485] Inter-dependency between { tree-sra, ABI, inlining, loop-unrolling } leads to mis-optimization

[pinskia at gcc dot gnu.org]

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

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

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

--- Comment #12 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Dup.

*** This bug has been marked as a duplicate of bug 97009 ***