[PATCH] D81408: [builtins] Improve compatibility with 16 bit targets

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko added inline comments.


================
Comment at: compiler-rt/lib/builtins/fp_lib.h:49
 
-static __inline int rep_clz(rep_t a) { return __builtin_clz(a); }
+static __inline int rep_clz(rep_t a) { return clzsi(a); }
 
----------------
efriedma wrote:
> MaskRay wrote:
> > atrosinenko wrote:
> > > MaskRay wrote:
> > > > Why is this needed?
> > > 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](https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html) 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 `int`s 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:
> > > 
> > > ```lang=cpp,name=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.
> > 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.
> > Where is this clzsi defined?
> 
> compiler-rt/lib/builtins/int_types.h
Just to have these defines here for a reference:

```lang=c,name=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
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81408