[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anton Korobeynikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0ee439b705e8: [builtins] Change si_int to int in some helper 
declarations (authored by atrosinenko, committed by asl).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81285

Files:
  compiler-rt/lib/builtins/README.txt
  compiler-rt/lib/builtins/clzdi2.c
  compiler-rt/lib/builtins/clzsi2.c
  compiler-rt/lib/builtins/clzti2.c
  compiler-rt/lib/builtins/ctzdi2.c
  compiler-rt/lib/builtins/ctzsi2.c
  compiler-rt/lib/builtins/ctzti2.c
  compiler-rt/lib/builtins/ffsti2.c
  compiler-rt/lib/builtins/int_lib.h
  compiler-rt/lib/builtins/paritydi2.c
  compiler-rt/lib/builtins/paritysi2.c
  compiler-rt/lib/builtins/parityti2.c
  compiler-rt/lib/builtins/popcountsi2.c
  compiler-rt/lib/builtins/popcountti2.c
  compiler-rt/lib/builtins/powidf2.c
  compiler-rt/lib/builtins/powisf2.c
  compiler-rt/lib/builtins/powitf2.c
  compiler-rt/lib/builtins/powixf2.c
  compiler-rt/test/builtins/Unit/clzdi2_test.c
  compiler-rt/test/builtins/Unit/clzsi2_test.c
  compiler-rt/test/builtins/Unit/clzti2_test.c
  compiler-rt/test/builtins/Unit/ctzsi2_test.c
  compiler-rt/test/builtins/Unit/ctzti2_test.c
  compiler-rt/test/builtins/Unit/ffsti2_test.c
  compiler-rt/test/builtins/Unit/paritydi2_test.c
  compiler-rt/test/builtins/Unit/paritysi2_test.c
  compiler-rt/test/builtins/Unit/parityti2_test.c
  compiler-rt/test/builtins/Unit/popcountsi2_test.c
  compiler-rt/test/builtins/Unit/popcountti2_test.c
  compiler-rt/test/builtins/Unit/powidf2_test.c
  compiler-rt/test/builtins/Unit/powisf2_test.c
  compiler-rt/test/builtins/Unit/powitf2_test.c
  compiler-rt/test/builtins/Unit/powixf2_test.c

Index: compiler-rt/test/builtins/Unit/powixf2_test.c
===
--- compiler-rt/test/builtins/Unit/powixf2_test.c
+++ compiler-rt/test/builtins/Unit/powixf2_test.c
@@ -11,9 +11,9 @@
 
 // Returns: a ^ b
 
-COMPILER_RT_ABI long double __powixf2(long double a, si_int b);
+COMPILER_RT_ABI long double __powixf2(long double a, int b);
 
-int test__powixf2(long double a, si_int b, long double expected)
+int test__powixf2(long double a, int b, long double expected)
 {
 long double x = __powixf2(a, b);
 int correct = (x == expected) && (signbit(x) == signbit(expected));
@@ -58,9 +58,9 @@
 return 1;
 if (test__powixf2(0, 4, 0))
 return 1;
-if (test__powixf2(0, 0x7FFE, 0))
+if (test__powixf2(0, INT_MAX - 1, 0))
 return 1;
-if (test__powixf2(0, 0x7FFF, 0))
+if (test__powixf2(0, INT_MAX, 0))
 return 1;
 
 if (test__powixf2(-0., 1, -0.))
@@ -71,9 +71,9 @@
 return 1;
 if (test__powixf2(-0., 4, 0))
 return 1;
-if (test__powixf2(-0., 0x7FFE, 0))
+if (test__powixf2(-0., INT_MAX - 1, 0))
 return 1;
-if (test__powixf2(-0., 0x7FFF, -0.))
+if (test__powixf2(-0., INT_MAX, -0.))
 return 1;
 
 if (test__powixf2(1, 1, 1))
@@ -84,9 +84,9 @@
 return 1;
 if (test__powixf2(1, 4, 1))
 return 1;
-if (test__powixf2(1, 0x7FFE, 1))
+if (test__powixf2(1, INT_MAX - 1, 1))
 return 1;
-if (test__powixf2(1, 0x7FFF, 1))
+if (test__powixf2(1, INT_MAX, 1))
 return 1;
 
 if (test__powixf2(INFINITY, 1, INFINITY))
@@ -97,9 +97,9 @@
 return 1;
 if (test__powixf2(INFINITY, 4, INFINITY))
 return 1;
-if (test__powixf2(INFINITY, 0x7FFE, INFINITY))
+if (test__powixf2(INFINITY, INT_MAX - 1, INFINITY))
 return 1;
-if (test__powixf2(INFINITY, 0x7FFF, INFINITY))
+if (test__powixf2(INFINITY, INT_MAX, INFINITY))
 return 1;
 
 if (test__powixf2(-INFINITY, 1, -INFINITY))
@@ -110,9 +110,9 @@
 return 1;
 if (test__powixf2(-INFINITY, 4, INFINITY))
 return 1;
-if (test__powixf2(-INFINITY, 0x7FFE, INFINITY))
+if (test__powixf2(-INFINITY, INT_MAX - 1, INFINITY))
 return 1;
-if (test__powixf2(-INFINITY, 0x7FFF, -INFINITY))
+if (test__powixf2(-INFINITY, INT_MAX, -INFINITY))
 return 1;
 
 if (test__powixf2(0, -1, INFINITY))
@@ -123,11 +123,11 @@
 return 1;
 if (test__powixf2(0, -4, INFINITY))
 return 1;
-if (test__powixf2(0, 0x8002, INFINITY))
+if (test__powixf2(0, INT_MIN + 2, INFINITY))
 return 1;
-if (test__powixf2(0, 0x8001, INFINITY))
+if (test__powixf2(0, INT_MIN + 1, INFINITY))
 return 1;
-if (test__powixf2(0, 0x8000, INFINITY))
+if (test__powixf2(0, INT_MIN, INFINITY))
 return 1;
 
 if (test__powixf2(-0., -1, -INFINITY))
@@ -138,11 +138,11 @@
 return 1;
 if (test__powixf2(-0., -4, INFINITY))
 return 1;
-if (test__powixf2(-0., 0x8002, INFINITY))
+if (test__powixf2(-0., INT_MIN + 2, INFINITY))
 return 1;
-if (test__powixf2(-0., 

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko added a comment.

Thank you!

In D81285#2110102 , @MaskRay wrote:

> > This patch changes types of some integer function arguments or return 
> > values from si_int to the default int type (typedefed to native_int to make 
> > it obvious this is intentional) to make it more compatible with libgcc.
>
> Please drop `native_int` from the description since the code has been 
> adjusted.


Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81285



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.

> This patch changes types of some integer function arguments or return values 
> from si_int to the default int type (typedefed to native_int to make it 
> obvious this is intentional) to make it more compatible with libgcc.

Please drop `native_int` from the description since the code has been adjusted.

I don't use any sizeof(int)==2 platform and can't verify whether the patch is 
good on MSP430 or AVR. Hope @aykevl con verify the correctness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81285



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM.  (Please wait a few days before merging to see if anyone else has 
comments.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81285



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ayke via Phabricator via cfe-commits]

aykevl added a comment.

In D81285#2102948 , @atrosinenko wrote:

> On renaming fixed width integer types to their traditional names: I would 
> prefer sending such patch afterwards, it would probably be as simple as just 
> running `sed --in-place` several times.


Yes definitely. It seems best to me to separate refactorings from functional 
changes. If it should indeed be done, I'm not a maintainer of compiler-rt.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81285



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko updated this revision to Diff 271963.
atrosinenko added a comment.

Replaced `native_int` by plain `int`, as @aykevl suggested.

On one hand, I have no specific preference for using some descriptive name such 
as `native_int`, `default_int`, etc. (that should suggest it was chosen 
intentionally, not because "I had no better idea") or just use standard `int` 
instead (supposing special names would be more confusing than explaining).

On the other hand, I aggree with @aykevl that replacing machine mode-related 
names (locally typedef'ed `[sdt][ui]_int`) with widely known type names such as 
uint32_t that should be equivalent by the definition according to GCC 
documentation  would 
simplify understanding of the builtins source code. And after such change, 
using `native_int` for just `int` and absolutely traditional names for most 
other types would be quite strange.

On renaming fixed width integer types to their traditional names: I would 
prefer sending such patch afterwards, it would probably be as simple as just 
running `sed --in-place` several times.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81285

Files:
  compiler-rt/lib/builtins/README.txt
  compiler-rt/lib/builtins/clzdi2.c
  compiler-rt/lib/builtins/clzsi2.c
  compiler-rt/lib/builtins/clzti2.c
  compiler-rt/lib/builtins/ctzdi2.c
  compiler-rt/lib/builtins/ctzsi2.c
  compiler-rt/lib/builtins/ctzti2.c
  compiler-rt/lib/builtins/ffsti2.c
  compiler-rt/lib/builtins/int_lib.h
  compiler-rt/lib/builtins/paritydi2.c
  compiler-rt/lib/builtins/paritysi2.c
  compiler-rt/lib/builtins/parityti2.c
  compiler-rt/lib/builtins/popcountsi2.c
  compiler-rt/lib/builtins/popcountti2.c
  compiler-rt/lib/builtins/powidf2.c
  compiler-rt/lib/builtins/powisf2.c
  compiler-rt/lib/builtins/powitf2.c
  compiler-rt/lib/builtins/powixf2.c
  compiler-rt/test/builtins/Unit/clzdi2_test.c
  compiler-rt/test/builtins/Unit/clzsi2_test.c
  compiler-rt/test/builtins/Unit/clzti2_test.c
  compiler-rt/test/builtins/Unit/ctzsi2_test.c
  compiler-rt/test/builtins/Unit/ctzti2_test.c
  compiler-rt/test/builtins/Unit/ffsti2_test.c
  compiler-rt/test/builtins/Unit/paritydi2_test.c
  compiler-rt/test/builtins/Unit/paritysi2_test.c
  compiler-rt/test/builtins/Unit/parityti2_test.c
  compiler-rt/test/builtins/Unit/popcountsi2_test.c
  compiler-rt/test/builtins/Unit/popcountti2_test.c
  compiler-rt/test/builtins/Unit/powidf2_test.c
  compiler-rt/test/builtins/Unit/powisf2_test.c
  compiler-rt/test/builtins/Unit/powitf2_test.c
  compiler-rt/test/builtins/Unit/powixf2_test.c

Index: compiler-rt/test/builtins/Unit/powixf2_test.c
===
--- compiler-rt/test/builtins/Unit/powixf2_test.c
+++ compiler-rt/test/builtins/Unit/powixf2_test.c
@@ -11,9 +11,9 @@
 
 // Returns: a ^ b
 
-COMPILER_RT_ABI long double __powixf2(long double a, si_int b);
+COMPILER_RT_ABI long double __powixf2(long double a, int b);
 
-int test__powixf2(long double a, si_int b, long double expected)
+int test__powixf2(long double a, int b, long double expected)
 {
 long double x = __powixf2(a, b);
 int correct = (x == expected) && (signbit(x) == signbit(expected));
@@ -58,9 +58,9 @@
 return 1;
 if (test__powixf2(0, 4, 0))
 return 1;
-if (test__powixf2(0, 0x7FFE, 0))
+if (test__powixf2(0, INT_MAX - 1, 0))
 return 1;
-if (test__powixf2(0, 0x7FFF, 0))
+if (test__powixf2(0, INT_MAX, 0))
 return 1;
 
 if (test__powixf2(-0., 1, -0.))
@@ -71,9 +71,9 @@
 return 1;
 if (test__powixf2(-0., 4, 0))
 return 1;
-if (test__powixf2(-0., 0x7FFE, 0))
+if (test__powixf2(-0., INT_MAX - 1, 0))
 return 1;
-if (test__powixf2(-0., 0x7FFF, -0.))
+if (test__powixf2(-0., INT_MAX, -0.))
 return 1;
 
 if (test__powixf2(1, 1, 1))
@@ -84,9 +84,9 @@
 return 1;
 if (test__powixf2(1, 4, 1))
 return 1;
-if (test__powixf2(1, 0x7FFE, 1))
+if (test__powixf2(1, INT_MAX - 1, 1))
 return 1;
-if (test__powixf2(1, 0x7FFF, 1))
+if (test__powixf2(1, INT_MAX, 1))
 return 1;
 
 if (test__powixf2(INFINITY, 1, INFINITY))
@@ -97,9 +97,9 @@
 return 1;
 if (test__powixf2(INFINITY, 4, INFINITY))
 return 1;
-if (test__powixf2(INFINITY, 0x7FFE, INFINITY))
+if (test__powixf2(INFINITY, INT_MAX - 1, INFINITY))
 return 1;
-if (test__powixf2(INFINITY, 0x7FFF, INFINITY))
+if (test__powixf2(INFINITY, INT_MAX, INFINITY))
 return 1;
 
 if (test__powixf2(-INFINITY, 1, -INFINITY))
@@ -110,9 +110,9 @@
 return 1;
 if (test__powixf2(-INFINITY, 4, INFINITY))
 return 1;
-if (test__powixf2(-INFINITY, 0x7FFE, INFINITY))
+if (test__powixf2(-INFINITY, INT_MAX - 1, INFINITY))
 return 1;
- 

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

[Ayke via Phabricator via cfe-commits]

aykevl added a comment.

I need to recompile LLVM to test this patch so it might take a while.

In D81285#2082517 , @atrosinenko wrote:

> In D81285#2082394 , @aykevl wrote:
>
> > Also note that the libgcc documentation does not always reflect the real 
> > world. For example, `__divmodsi4` on AVR libgcc has a very different 
> > signature: it returns both the division result and the remainder in 
> > registers.
>
>
> Do you mean some special calling convention not used outside the 
> `libgcc`/`clang_rt`? MSP430 has one as well. And I still have to decide how 
> to express for //some of// generic C implementations that they should use 
> that special calling convention on MSP430 without cluttering the sources with 
> `O(target count)` complexity. :) Meanwhile, some hacks do exist for ARM 
> target already.


I don't mean a separate calling convention, although AVR has that as well. 
Rather, that the builtin has a different function signature. This signature is 
a bit hard to express in C so you might consider it a different ABI I guess. I 
implemented it in C by packing the two return values in a single `uint64_t`.




Comment at: compiler-rt/lib/builtins/int_lib.h:112
 
-uint32_t __inline __builtin_ctz(uint32_t value) {
+int __inline __builtin_ctz(uint32_t value) {
   unsigned long trailing_zero = 0;

atrosinenko wrote:
> aykevl wrote:
> > Why `int` and not `native_int` here?
> Just to use more "textually identical" prototype as for an actual 
> `__builtin_ctz` from GCC. On the other hand, such dilemma could be one of 
> arguments against `native_int`.
Yes, that makes sense. It is actually defined with `int` in [the official 
documentation](https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html), and 
because this is at the C language level and not at the ABI/target level, it's 
not one of the "int but really int32_t" types.

That said, I think the parameter should also be `unsigned int` instead of 
`uint32_t`. And the same goes for the other types below. Although, in practice, 
it doesn't really matter as it's all MSVC with 32-bit int everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81285



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko added inline comments.



Comment at: compiler-rt/lib/builtins/int_lib.h:112
 
-uint32_t __inline __builtin_ctz(uint32_t value) {
+int __inline __builtin_ctz(uint32_t value) {
   unsigned long trailing_zero = 0;

aykevl wrote:
> Why `int` and not `native_int` here?
Just to use more "textually identical" prototype as for an actual 
`__builtin_ctz` from GCC. On the other hand, such dilemma could be one of 
arguments against `native_int`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81285



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko added a comment.

In D81285#2082394 , @aykevl wrote:

> I'm not sure whether `native_int` is any clearer than just `int`. I'm afraid 
> it only introduces more complexity ("What's `native_int`? Oh, it's just 
> `int`").


I'm not particularly insisting on the `native_int`, `default_int` or something 
similar. The decision to use some custom `typedef` is definitely an RFC here. 
My initial point was it explicitly notifies the reader "this type is not 
because I had not a better idea, it was specified intentionally as `int`".

> Perhaps a controversial idea: what about changing to use stdint.h types?
>  `si_int` -> `int32_t`
>  `su_int` -> `uint32_t`
>  `di_int` -> `int64_t`
> etc
>  These types are clearly defined and immediately recognizable.

I definitely support using the traditional integer types from `stdint.h` 
instead of some locally defined and not generally known ones.

> The meaning is, as far as I can see, exactly the same (`si_int` etc seems to 
> be a leftover from GCC internal naming conventions, such as `SImode`).

... I would just recheck (myself) that these three modes are identical "by 
definition" and replace them with `sed` :)

> Also note that the libgcc documentation does not always reflect the real 
> world. For example, `__divmodsi4` on AVR libgcc has a very different 
> signature: it returns both the division result and the remainder in registers.

Do you mean some special calling convention not used outside the 
`libgcc`/`clang_rt`? MSP430 has one as well. And I still have to decide how to 
express for //some of// generic C implementations that they should use that 
special calling convention on MSP430 without cluttering the sources with 
`O(target count)` complexity. :) Meanwhile, some hacks do exist for ARM target 
already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81285



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ayke via Phabricator via cfe-commits]

aykevl added a comment.

I'm not sure whether `native_int` is any clearer than just `int`. I'm afraid it 
only introduces more complexity ("What's `native_int`? Oh, it's just `int`").

Perhaps a controversial idea: what about changing to use stdint.h types?
`si_int` -> `int32_t`
`su_int` -> `uint32_t`
`di_int` -> `int64_t`
etc
These types are clearly defined and immediately recognizable. The meaning is, 
as far as I can see, exactly the same (`si_int` etc seems to be a leftover from 
GCC internal naming conventions, such as `SImode`).

Also note that the libgcc documentation does not always reflect the real world. 
For example, `__divmodsi4` on AVR libgcc has a very different signature: it 
returns both the division result and the remainder in registers.




Comment at: compiler-rt/lib/builtins/int_lib.h:112
 
-uint32_t __inline __builtin_ctz(uint32_t value) {
+int __inline __builtin_ctz(uint32_t value) {
   unsigned long trailing_zero = 0;

Why `int` and not `native_int` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81285



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko created this revision.
atrosinenko added a reviewer: howard.hinnant.
Herald added a project: Sanitizers.
Herald added a subscriber: Sanitizers.

This patch changes types of some integer function arguments or return values 
from `si_int` to the default `int` type (`typedef`ed to `native_int` to make it 
obvious this is intentional) to make it more compatible with `libgcc`.

The compiler-rt/lib/builtins/README.txt has a link to the libgcc specification 
. This specification 
has an explicit note on `int`, `float` and other such types being just 
illustrations in some cases while the actual types are expressed with machine 
modes.

Such usage of always-32-bit-wide integer type may lead to issues on 16-bit 
platforms such as MSP430. Provided libgcc2.h 
 
can be used as a reference for all targets supported by the libgcc, this patch 
fixes some existing differences in helper declarations.

This patch is expected to not change behavior at all for targets with 32-bit 
`int` type.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81285

Files:
  compiler-rt/lib/builtins/README.txt
  compiler-rt/lib/builtins/clzdi2.c
  compiler-rt/lib/builtins/clzsi2.c
  compiler-rt/lib/builtins/clzti2.c
  compiler-rt/lib/builtins/ctzdi2.c
  compiler-rt/lib/builtins/ctzsi2.c
  compiler-rt/lib/builtins/ctzti2.c
  compiler-rt/lib/builtins/ffsdi2.c
  compiler-rt/lib/builtins/ffssi2.c
  compiler-rt/lib/builtins/ffsti2.c
  compiler-rt/lib/builtins/int_lib.h
  compiler-rt/lib/builtins/int_types.h
  compiler-rt/lib/builtins/paritydi2.c
  compiler-rt/lib/builtins/paritysi2.c
  compiler-rt/lib/builtins/parityti2.c
  compiler-rt/lib/builtins/popcountdi2.c
  compiler-rt/lib/builtins/popcountsi2.c
  compiler-rt/lib/builtins/popcountti2.c
  compiler-rt/lib/builtins/powidf2.c
  compiler-rt/lib/builtins/powisf2.c
  compiler-rt/lib/builtins/powitf2.c
  compiler-rt/lib/builtins/powixf2.c
  compiler-rt/test/builtins/Unit/clzdi2_test.c
  compiler-rt/test/builtins/Unit/clzsi2_test.c
  compiler-rt/test/builtins/Unit/clzti2_test.c
  compiler-rt/test/builtins/Unit/ctzdi2_test.c
  compiler-rt/test/builtins/Unit/ctzsi2_test.c
  compiler-rt/test/builtins/Unit/ctzti2_test.c
  compiler-rt/test/builtins/Unit/ffsdi2_test.c
  compiler-rt/test/builtins/Unit/ffssi2_test.c
  compiler-rt/test/builtins/Unit/ffsti2_test.c
  compiler-rt/test/builtins/Unit/paritydi2_test.c
  compiler-rt/test/builtins/Unit/paritysi2_test.c
  compiler-rt/test/builtins/Unit/parityti2_test.c
  compiler-rt/test/builtins/Unit/popcountdi2_test.c
  compiler-rt/test/builtins/Unit/popcountsi2_test.c
  compiler-rt/test/builtins/Unit/popcountti2_test.c
  compiler-rt/test/builtins/Unit/powidf2_test.c
  compiler-rt/test/builtins/Unit/powisf2_test.c
  compiler-rt/test/builtins/Unit/powitf2_test.c
  compiler-rt/test/builtins/Unit/powixf2_test.c

Index: compiler-rt/test/builtins/Unit/powixf2_test.c
===
--- compiler-rt/test/builtins/Unit/powixf2_test.c
+++ compiler-rt/test/builtins/Unit/powixf2_test.c
@@ -22,9 +22,9 @@
 
 // Returns: a ^ b
 
-COMPILER_RT_ABI long double __powixf2(long double a, si_int b);
+COMPILER_RT_ABI long double __powixf2(long double a, native_int b);
 
-int test__powixf2(long double a, si_int b, long double expected)
+int test__powixf2(long double a, native_int b, long double expected)
 {
 long double x = __powixf2(a, b);
 int correct = (x == expected) && (signbit(x) == signbit(expected));
@@ -69,9 +69,9 @@
 return 1;
 if (test__powixf2(0, 4, 0))
 return 1;
-if (test__powixf2(0, 0x7FFE, 0))
+if (test__powixf2(0, INT_MAX - 1, 0))
 return 1;
-if (test__powixf2(0, 0x7FFF, 0))
+if (test__powixf2(0, INT_MAX, 0))
 return 1;
 
 if (test__powixf2(-0., 1, -0.))
@@ -82,9 +82,9 @@
 return 1;
 if (test__powixf2(-0., 4, 0))
 return 1;
-if (test__powixf2(-0., 0x7FFE, 0))
+if (test__powixf2(-0., INT_MAX - 1, 0))
 return 1;
-if (test__powixf2(-0., 0x7FFF, -0.))
+if (test__powixf2(-0., INT_MAX, -0.))
 return 1;
 
 if (test__powixf2(1, 1, 1))
@@ -95,9 +95,9 @@
 return 1;
 if (test__powixf2(1, 4, 1))
 return 1;
-if (test__powixf2(1, 0x7FFE, 1))
+if (test__powixf2(1, INT_MAX - 1, 1))
 return 1;
-if (test__powixf2(1, 0x7FFF, 1))
+if (test__powixf2(1, INT_MAX, 1))
 return 1;
 
 if (test__powixf2(INFINITY, 1, INFINITY))
@@ -108,9 +108,9 @@
 return 1;
 if (test__powixf2(INFINITY, 4, INFINITY))
 return 1;
-if (test__powixf2(INFINITY, 0x7FFE, INFINITY))
+if (test__powixf2(INFINITY, INT_MAX - 1, INFINITY))
 return 1;
-if (test__powixf2(INFINITY, 0x7FFF, INFINITY))
+if (test__powixf2(INFINITY, INT_MAX, INFINITY))
 return 1;