[builtins] Improve compatibility with 16 bit targets

[atrosinenko]

Summary

Some parts of existing codebase assume the default int type to be (at least) 32 bit wide. On 16 bit targets such as MSP430 this may cause Undefined Behavior or results being defined but incorrect.

[MaskRay]

compiler-rt/lib/builtins/fp_lib.h

49

Why is this needed?

[atrosinenko]

compiler-rt/lib/builtins/fp_lib.h

49

First of all, I will compile the examples with gcc because right now I most probably don't have a TI sysroot for MSP430 compatible with the mainline clang and testing with my locally patched llvm+clang seems even less correct than with gcc.

The problem is that the definition, according to the gcc documentation is int __builtin_clz (unsigned int x). This builtin is defined with the "default size" int argument, not uint32_t or int32_t. Yes, there are ints in the specifications that are not exactly int but something "expected to be int" actually expressed via machine modes. But here it looks like it is actually an int:

clzsi_test.c

#include <stdio.h>
#include <stdint.h>

typedef uint32_t rep_t;

static int rep_clz(rep_t a) { return __builtin_clz(a); }

int main()
{
  printf("sizeof(int) = %d\n", (int)sizeof(int));
  printf("first:  %d\n", rep_clz(0x1000));
  printf("second: %d\n", rep_clz(0x10000));
  printf("third:  %d\n", rep_clz(0x100000));
}

When compiled and launched on x86_64 Linux host, it prints the expected results:

$ gcc -Wall clzsi_test.c -o clzsi_test && ./clzsi_test 
sizeof(int) = 4
first:  19
second: 15
third:  11

Let's compile and run it with

$ $sysroot/bin/msp430-elf-gcc --version
msp430-elf-gcc (Mitto Systems Limited - msp430-gcc 8.3.1.25) 8.3.1
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Then we get the following (the -msim option is necessary to run the compiled binary with msp430-elf-run simulator):

$ $sysroot/bin/msp430-elf-gcc -msim -Wall clzsi_test.c -o clzsi_test && $sysroot/bin/msp430-elf-run ./clzsi_test 
sizeof(int) = 2
first:  3
second: 16
third:  16

So, these are expected results for clz(i16 arg) (with the third line being an UB at all) but not the results expected for rep_t-sized argument. (When I comment out the third printf, other output that now is expected to be UB-free remains unchanged)

When I change rep_clz to use __builtin_clzl, the output on MSP430 simulator differs from the host one just in the sizeof... line, as expected.

sizeof(int) = 2
first:  19
second: 15
third:  11

Of course, now it will output not the expected values on x86_64, so using clzsi instead of a hardcoded name.

[MaskRay]

compiler-rt/lib/builtins/fp_lib.h

49

Where is this clzsi defined? It seems that clzsi is provided by either __builtin_clz or __builtin_clzl, but the desired version is a customized one.

[efriedma]

compiler-rt/lib/builtins/fp_lib.h

49

Where is this clzsi defined?

compiler-rt/lib/builtins/int_types.h

[atrosinenko]

compiler-rt/lib/builtins/fp_lib.h

49

Just to have these defines here for a reference:

compiler-rt/lib/builtins/int_types.h

typedef int32_t si_int;
typedef uint32_t su_int;
#if UINT_MAX == 0xFFFFFFFF
#define clzsi __builtin_clz
#define ctzsi __builtin_ctz
#elif ULONG_MAX == 0xFFFFFFFF
#define clzsi __builtin_clzl
#define ctzsi __builtin_ctzl
#else
#error could not determine appropriate clzsi macro for this system
#endif

[aykevl]

I tested this patch against my local testing system to make sure it didn't break anything, and I get the same number of failures (most of which are due to a missing complex.h file, which is unrelated). So it looks fine from my point of view, although I know not enough about this code to LGTM it.
Note that double float (float128) doesn't work in my setup, so I can't test changes related to that.

[uabelho]

Our out-of-tree target also have 16 bit ints and we've had to change a whole bunch of builtin_clz to clzsi and builtin_ctz to ctzsi to make things work so this patch is a step in the right direction for us too.

[atrosinenko]

Rebase onto working upstream commit to (hopefully) make tests pass for my patch.

[atrosinenko]

Ping.

Those flaky test failures seems to be due to ld.lld being not built from source as part of testing compiler-rt/-only patches.

[efriedma]

Those flaky test failures seems to be due to ld.lld being not built from source as part of testing compiler-rt/-only patches.

That should be something we can fix in the build system. compiler-rt/test/CMakeLists.txt has a list of executables which the tests depend on. If that list isn't complete, it should be updated.

[atrosinenko]

In D81408#2104572, @efriedma wrote:

Those flaky test failures seems to be due to ld.lld being not built from source as part of testing compiler-rt/-only patches.

That should be something we can fix in the build system. compiler-rt/test/CMakeLists.txt has a list of executables which the tests depend on. If that list isn't complete, it should be updated.

@efriedma

Agree, but another question: is it acceptable to have them disabled when retesting compiler-rt? So, maybe lld subproject have to be explicitly enabled as well for compiler-rt-only tests...

@goncharov

Meanwhile, looking at the latest Buildkite build for this review, we have:

artifacts/CMakeCache.txt (Linux)

...
LLVM_ENABLE_PROJECTS:STRING=clang;compiler-rt;llvm
...

and

artifacts/CMakeCache.txt (Windows)

...
LLVM_ENABLE_PROJECTS:STRING=clang;llvm
...

So, isn't Windows build here just wasting CPU time?

Another my concern here is that the build plan pre-merge checks (buildkite) is expressed with just a single "Make HTTP request" step with "When Complete: Wait For Message" (unlike the pre-merge checks used for the initial upload). So, this build failed like this (see the message tab):

216557	merge_guards_bot	fail	Consumed	Thu, Jun 18, 16:13
216556	merge_guards_bot	working		Thu, Jun 18, 16:13
216538	merge_guards_bot	working		Thu, Jun 18, 15:51

In D81282: [builtins] Move more float128-related helpers to GENERIC_TF_SOURCES list, on the other hand, there is seemingly passed build. Its messages are

216370	merge_guards_bot	fail		Thu, Jun 18, 13:28
216365	merge_guards_bot	working		Thu, Jun 18, 13:25
216343	merge_guards_bot	pass	Consumed	Thu, Jun 18, 12:53
216342	merge_guards_bot	working		Thu, Jun 18, 12:52

So, it looks like now the compiler-rt-only pre-merge check cannot pass at all, but can be marked Passed on re-upload only based on what builder (Linux or Windows) will send its reply first.

[efriedma]

LGTM

We don't need to block this on the pre-merge checks.

[goncharov]

In D81408#2105188, @atrosinenko wrote:

@goncharov

Meanwhile, looking at the latest Buildkite build for this review, we have:

artifacts/CMakeCache.txt (Linux)

...
LLVM_ENABLE_PROJECTS:STRING=clang;compiler-rt;llvm
...

and

artifacts/CMakeCache.txt (Windows)

...
LLVM_ENABLE_PROJECTS:STRING=clang;llvm
...

So, isn't Windows build here just wasting CPU time?

@atrosinenko: could you please clarify what exactly is wrong? Windows not building 'compiler-rt'?

Another my concern here is that the build plan pre-merge checks (buildkite) is expressed with just a single "Make HTTP request" step with "When Complete: Wait For Message" (unlike the pre-merge checks used for the initial upload). So, this build failed like this (see the message tab):

216557	merge_guards_bot	fail	Consumed	Thu, Jun 18, 16:13
216556	merge_guards_bot	working		Thu, Jun 18, 16:13
216538	merge_guards_bot	working		Thu, Jun 18, 15:51

In D81282: [builtins] Move more float128-related helpers to GENERIC_TF_SOURCES list, on the other hand, there is seemingly passed build. Its messages are

216370	merge_guards_bot	fail		Thu, Jun 18, 13:28
216365	merge_guards_bot	working		Thu, Jun 18, 13:25
216343	merge_guards_bot	pass	Consumed	Thu, Jun 18, 12:53
216342	merge_guards_bot	working		Thu, Jun 18, 12:52

So, it looks like now the compiler-rt-only pre-merge check cannot pass at all, but can be marked Passed on re-upload only based on what builder (Linux or Windows) will send its reply first.

Yes, there is a single request to buildkite and a single response that happens at "report" step after both windows and linux build are done, e.g. https://buildkite.com/llvm-project/premerge-checks/builds/898#7efe45eb-ac96-40aa-ba91-494a5812ec88

The latter "successfull" build was a temporary misconfiguration of the build scripts. 1. I believe I have restarted all affected builds but clearly missed some. 2. I have created https://github.com/google/llvm-premerge-checks/issues/206 to prevent this from happening,

[atrosinenko]

could you please clarify what exactly is wrong? Windows not building 'compiler-rt'?

Yes. Maybe I misunderstood the intention. It just looked quite strange to me that when only the compiler-rt/ subtree of the repository was changed, pre-merge check was optimizing build by dropping all unrelated subprojects - and compiler-rt was dropped as well.

compiler-rt/lib/builtins/floatdidf.c

if (sd > DBL_MANT_DIG) {
     }
     // a is now rounded to DBL_MANT_DIG bits
   } else {
     a <<= (DBL_MANT_DIG - sd);
     // a is now rounded to DBL_MANT_DIG bits
   }
   double_bits fb;
   fb.u.s.high = ((su_int)s & 0x80000000) |        // sign
-                ((e + 1023) << 20) |              // exponent
+                ((su_int)(e + 1023) << 20) |      // exponent
                 ((su_int)(a >> 32) & 0x000FFFFF); // mantissa-high
   fb.u.s.low = (su_int)a;                         // mantissa-low
   return fb.f;
 }
 #endif
 
 #if defined(__ARM_EABI__)
 #if defined(COMPILER_RT_ARMHF_TARGET)
 AEABI_RTABI double __aeabi_l2d(di_int a) { return __floatdidf(a); }
 #else
 COMPILER_RT_ALIAS(__floatdidf, __aeabi_l2d)
 #endif
 #endif

compiler-rt/lib/builtins/floatundidf.c

if (a & ((du_int)1 << DBL_MANT_DIG)) {
       ++e;
     }
     // a is now rounded to DBL_MANT_DIG bits
   } else {
     a <<= (DBL_MANT_DIG - sd);
     // a is now rounded to DBL_MANT_DIG bits
   }
   double_bits fb;
-  fb.u.s.high = ((e + 1023) << 20) |              // exponent
+  fb.u.s.high = ((su_int)(e + 1023) << 20) |      // exponent
                 ((su_int)(a >> 32) & 0x000FFFFF); // mantissa-high
   fb.u.s.low = (su_int)a;                         // mantissa-low
   return fb.f;
 }
 #endif
 
 #if defined(__ARM_EABI__)
 #if defined(COMPILER_RT_ARMHF_TARGET)
 AEABI_RTABI double __aeabi_ul2d(du_int a) { return __floatundidf(a); }
 #else
 COMPILER_RT_ALIAS(__floatundidf, __aeabi_ul2d)
 #endif
 #endif

compiler-rt/lib/builtins/fp_lib.h

 #if defined SINGLE_PRECISION
 
 typedef uint32_t rep_t;
 typedef int32_t srep_t;
 typedef float fp_t;
 #define REP_C UINT32_C
 #define significandBits 23
 
-static __inline int rep_clz(rep_t a) { return __builtin_clz(a); }
+static __inline int rep_clz(rep_t a) { return clzsi(a); }
 
 // 32x32 --> 64 bit multiply
 static __inline void wideMultiply(rep_t a, rep_t b, rep_t *hi, rep_t *lo) {
   const uint64_t product = (uint64_t)a * b;
   *hi = product >> 32;
   *lo = product;
 }
 COMPILER_RT_ABI fp_t __addsf3(fp_t a, fp_t b);