[PATCH] D81285: [builtins] Change si_int to int in some helper declarations

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

Hi!

My users have found problems with miscompiles that seem to originate from this patch.

As a background, my target has 16-bit int, and 32-bit long. And the calling convention is not that an i16 is passed as the low bits in a 32-bit register that would be used for an i32 argument.
Thus, when the bultins are built for my target, and the signature doesn't specify the exact bit-width using `si_int`, but rather using `int`, then the lib function will expect to get a 16-bit input.

The problem is that opt/llc does not know about the size of `int` etc. But opt (SimplifyLibCalls) and llc (LegalizeDAG) can rewrite things into using libcalls to for example `__powisf2`. But when it generate those calls it will use a function signature with `int` being mapped to `i32`.

Afaict the change that made sure the size of si_int was respected (`typedef  int32_t si_int;`) was a nice change.
But I don't really understand why some types where changed to C-types instead of the Machine Mode types. Specially since you also quote that `int` only is used as an illustration but the real implementation should use the Machine Modes.

The miscompile found was when using `pow(x, (double)2u)` together with `-ffast-math`. opt rewrites it into calling `@llvm.powi.f64(double, i32)`, and later LegalizeDAG will rewrite that into a libcall to `__powidf2(double , i32)`, although that call is wrong if the lib function is compiler as `__powidf2(double , i16)`.

Btw, I've found similar problems in for example SimplifyLibCall, that for example could rewrite `powf(2.0, itofp(x))` to `ldexpf(1.0, x)`. The second argument of ldexpf is an `int` according to math.h, but LLVM does not really consider that when doing the transform so it will use i32 in the call (while the lib in our case expect to receive an i16).

So it is kind of a mess. But I figure at least these "GCC low-level runtime library" kind of calls should use the Machine Modes? Or do we need to match up with the types in https://gcc.gnu.org/git/?p=gcc.git;a=blob_plain;f=libgcc/libgcc2.h;hb=HEAD ? If so, then this patch that made changes to compiler-rt (which is what we use for those builtins) isn't enough and we need to teach opt/llc to take the size of int into account when generating certain lib calls.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81285/new/

https://reviews.llvm.org/D81285