[Bug c/96482] New: Combination of -finline-small-functions and ipa-cp optimisations causes incorrect values being passed to a function

[yevh.kolesnikov at gmail dot com]

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

Bug ID: 96482
   Summary: Combination of -finline-small-functions and ipa-cp
optimisations causes incorrect values being passed to
a function
   Product: gcc
   Version: 11.0
Status: UNCONFIRMED
  Severity: normal
  Priority: P3
 Component: c
  Assignee: unassigned at gcc dot gnu.org
  Reporter: yevh.kolesnikov at gmail dot com
  Target Milestone: ---

The bug was originally reported in mesa:
https://gitlab.freedesktop.org/mesa/mesa/-/issues/3239

Combination of LTO and O3 caused a crash on replaying a video with mpv.
Backtrace showed pretty unrealistic values passed to a function. I reduced a
number of necessary optimisations to:

+ -O1
+ -finline-small-functions
+ -fipa-bit-cp
+ -fipa-cp
+ LTO

It happens with gcc 10.1.0, but not with gcc 9. I bisected the problem to
c7ac9a0c7e3916f192ad41227e16238fd1fa2fbf.

I wasn't able to produce a minimal reproducer, and even though a preprocessed
file doesn't trigger the bug for me (I assume, LTO is essential for this bug),
I'm attaching it anyway.

Looking at the produced assembler, though, shows that it goes wrong somewhere
around a call to addr_to_index at
https://gitlab.freedesktop.org/mesa/mesa/-/blob/7f25f1f106728f0046672adf9c9ed79185265ea5/src/compiler/nir/nir_lower_io.c#L870.
addr_to_index gets inlined and so is nir_channel, called from it.

Those two are pretty simple functions:

static nir_ssa_def *
addr_to_index(nir_builder *b, nir_ssa_def *addr,
  nir_address_format addr_format)
{
   if (addr_format == nir_address_format_32bit_index_offset) {
  assert(addr->num_components == 2);
  return nir_channel(b, addr, 0);
   } else if (addr_format == nir_address_format_vec2_index_32bit_offset) {
  assert(addr->num_components == 3);
  return nir_channels(b, addr, 0x3);
   } else {
  unreachable("bad address format for index");
   }
}

static inline nir_ssa_def *
nir_channel(nir_builder *b, nir_ssa_def *def, unsigned c)
{
   return nir_swizzle(b, def, &c, 1);
}

In the final assembly we end up with just a call to nir_swizzle:
movedx,r14d
movrsi,r15
movrdi,rbp
call   0x7fffd26e0ef9 

[Bug c/96482] Combination of -finline-small-functions and ipa-cp optimisations causes incorrect values being passed to a function

[yevh.kolesnikov at gmail dot com]

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

--- Comment #1 from Yevhenii Kolesnikov  ---
Created attachment 49003
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49003&action=edit
Preprocessed file (compressed)

Oh. Preprocessed file was actually too big. Attaching a compressed file.

[Bug ipa/96482] [10/11 Regression] Combination of -finline-small-functions and ipa-cp optimisations causes incorrect values being passed to a function since r279523

[yevh.kolesnikov at gmail dot com]

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

--- Comment #5 from Yevhenii Kolesnikov  ---
(In reply to Martin Liška from comment #3)
> Thank you for the report, I can take a look.
> Can you please provide steps how to build Mesa with -O3 and -flto?

mesa is configured with meson. LTO can be enabled with builtin option
-b_lto=true. Options can be supplied to the compiler with -Dc_args and
-Dcpp_args. So, to enable O3 it's -Dc_args="-O3" -Dcpp_args="-O3". You'll also
need to set -Dbuildtype=plain, or otherwise these options will be overwritten. 

So, the full build process will look like this:

meson \
-Dbuildtype=plain \
-Dvalgrind=false \
-Ddri-drivers=i965 \
-Dgallium-drivers=iris,swrast \
-Dvulkan-drivers= \
-Dgallium-omx="disabled" \
-Dplatforms=x11,drm,surfaceless \
-Dllvm=false \
-Db_lto=true \
-Dc_args="-g \
  -O3 \
  " \
-Dcpp_args="-g \
-O3 \
" \
--prefix=~/builds/mesa-bug \
./mesa-bug

ninja -C ./mesa-bug install

The aforementioned functions and assembly code then will be in
~/builds/mesa-bug/lib/dri/iris_dri.so

> About the addr_to_index function: am I right that unreachable is defined in
> your configuration as __builtin_unreachable?

To be precise, unreachable is defined like this:

#define unreachable(str)\
do {\
   assert(!str);\
   __builtin_unreachable(); \
} while (0)

I'm not sure why this one-time loop is necessary, to be honest.

> Can you please replace it with fprintf that prints the value?

The value of addr_format? It doesn't print anything, since this branch is never
hit. Moreover, it prevents addr_to_index from being inlined, so the crash goes
away. The value of addr_format in my case is 3, which corresponds to the
nir_address_format_32bit_index_offset enum. So, in my case, the first branch is
always hit.

[Bug ipa/96482] [10/11 Regression] Combination of -finline-small-functions and ipa-cp optimisations causes incorrect values being passed to a function since r279523

[yevh.kolesnikov at gmail dot com]

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

--- Comment #6 from Yevhenii Kolesnikov  ---
(In reply to Jan Hubicka from comment #4)
> that patch makes ccp to actually use the bit info ipa-cp determines. Before
> we used it only to detect pointer alignments if I remember correctly. So it
> looks like propagation bug uncovered by the change.  Smaller testcase or
> reproduction steps would be indeed welcome.

Unfortunately, I wasn't able to create a standalone reproducer yet. Even in
mesa it's sometimes finicky to catch this bug. For example, simply adding an
fprintf to aforementioned function addr_to_index, prevents it from inlining.  

As for the reproduction steps with mesa:

1. Build mesa as described in comment #5
2. LD_LIBRARY_PATH=$LD_LIBRARY_PATH:~/builds/mesa-bug/lib/ mpv any_video.mp4
3. mesa crashes. Backtrace will slightly differ depending on the driver in use.
So far, I've been describing behavior on "iris" driver (default driver for
Intel GPUs). I assume, similar optimisation patterns occur in several places,
when building mesa.