[Bug c/108439] New: incorrect optimization with -O2, -O3, -Os

[Bug c/108439] New: incorrect optimization with -O2, -O3, -Os

[georgmueller at gmx dot net]

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

            Bug ID: 108439
           Summary: incorrect optimization with -O2, -O3, -Os
           Product: gcc
           Version: 12.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: georgmueller at gmx dot net
  Target Milestone: ---

Created attachment 54292
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54292&action=edit
test case to trigger bug

When testing the crap8 (or crapwow) hashing funtion, we discovered a gcc
optimization problem with gcc from at least version 9.3.0 up to 12.2.1.

The attached program (t_crap8.c) miscompiles for -O2, -O3, -Os while testing
the crap8 hash function (same behavior with similar hash function crapwow,
which only has different parameters m and n in the hash function).

Implementation is, for example. available in the smhasher project.

The line "lb_key[0] = 0x1234;" gets ignored with -O2, -O3, -Os.

As described in the report, compiling with -fno-strict-aliasing solves the
problem, but the code is compiling correctly with clang at all optimization
levels and does not report an error.

[Bug c/108439] incorrect optimization with -O2, -O3, -Os

[georgmueller at gmx dot net]

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

--- Comment #1 from Georg Müller <georgmueller at gmx dot net> ---
Created attachment 54293
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54293&action=edit
reduced test case to give a compile warning, which seems to be incorrect

While trying to reduce the problem and reducing the array size of lb_key to ony
one member (t_crap8-1u64.c), it gives a compile error with -O2, -O3 and -Os
even if the only member gets initialized all the time with the line "lb_key[0]
= 0x1234;".

Also, this compiler warning and execution error is removed by compiling with
-fno-strict-aliasing, but again, this does not look right.

[Bug c/108439] incorrect optimization with -O2, -O3, -Os

[georgmueller at gmx dot net]

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

--- Comment #2 from Georg Müller <georgmueller at gmx dot net> ---
Just a small additional note: The dependency on argc in the loop filling the
array in main() is only there to avoid early optimization.

[Bug c/108439] incorrect optimization with -O2, -O3, -Os

[pinskia at gcc dot gnu.org]

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

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

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Use -fno-strict-aliasing, mayalias attribute, memcpy, etc as you are violating
C/C++ aliasing rules.
You do a store via uint64_t:
        lb_key[0] = 0x1234;

And then do loads via uint32_t:
  const uint32_t  m = 0x83d2e73b, n = 0x97e1cc59, *key4 = (const uint32_t
*)key;
...
    c8mix(key4[0]) c8mix(key4[1]) key4 += 2;

[Bug c/108439] incorrect optimization with -O2, -O3, -Os

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Georg Müller from comment #1) 
> Also, this compiler warning and execution error is removed by compiling with
> -fno-strict-aliasing, but again, this does not look right.

Why do you think that is not correct? The aliasing violation causing undefined
behavior is obvious from the stores and loads that happen from the source.

[Bug c/108439] incorrect optimization with -O2, -O3, -Os

[georgmueller at gmx dot net]

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

--- Comment #5 from Georg Müller <georgmueller at gmx dot net> ---
(In reply to Andrew Pinski from comment #4)
> (In reply to Georg Müller from comment #1) 
> > Also, this compiler warning and execution error is removed by compiling with
> > -fno-strict-aliasing, but again, this does not look right.
> 
> Why do you think that is not correct? The aliasing violation causing
> undefined behavior is obvious from the stores and loads that happen from the
> source.

main() only works with its own lb_key array. It is only modified before giving
it to crap8() - it does not handle the internals of the hashing function.

crap8() takes the key as a void pointer - no assumptions about what it gets and
only converts void* to  uint32_t*, what should be fine, or?

Why is there a warning of lb_key not being initialized in the second example,
even though its only member gets initialized twice? Via the same reference...

[Bug c/108439] incorrect optimization with -O2, -O3, -Os

[pinskia at gcc dot gnu.org]

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

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Aliasing rules are NOT a local thing and they are not about what the type of
the pointer is, rather than they are about the access after the casting. They
are also global ...

GCC used to have strict aliasing warnings which might warn during optimizations
but it had many false positives so it was tuned down a lot.
The rules are simple in most cases (there are complex parts to it when dealing
with unions and structures but more complex in the implementation of the
compiler).

Some reading on this subject:
https://developers.redhat.com/blog/2020/06/02/the-joys-and-perils-of-c-and-c-aliasing-part-1
https://gist.github.com/shafik/848ae25ee209f698763cffee272a58f8

There are many more papers about C/C++ aliasing rules if you want to go read up
on why GCC is doing the "correct" thing here.

[Bug c/108439] incorrect optimization with -O2, -O3, -Os

[georgmueller at gmx dot net]

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

--- Comment #7 from Georg Müller <georgmueller at gmx dot net> ---
Ok, but then why isn't there a single warning - even with -Wstrict-aliasing=1
and a completely misleading error message for the second, reduced test?