Page MenuHomePhabricator

[builtin] Fixed definitions of builtins that rely on the int/long long type is 32/64 bits

Authored by Ka-Ka on May 12 2019, 11:46 PM.



The definition of the builtins builtin_bswap32, builtin_bitreverse32, builtin_rotateleft32 and builtin_rotateright32 rely on that the int type is 32 bits wide on the target.
The defintions of the builtins builtin_bswap64, builtin_bitreverse64, builtin_rotateleft64, and builtin_rotateright64 rely on that the long long type is 64 bits wide.

On targets where this is not the case (e.g. AVR) clang will generate faulty code (wrong llvm assembler intrinsics).

This patch add support for using 'Z' (the int32_t type) in Bultins.def. The builtins above are changed to be based on the int32_t type instead of the int type, and the int64_t type instead of the long long type.

The AVR backend (experimental) have a native int type that is only 16 bits wide. The supplied testcase will therefore fail if running the testcase on trunk as clang will convert e.g. __builtin_bitreverse32 into llvm.bitreverse.i16 on AVR.

Diff Detail


Event Timeline

Ka-Ka created this revision.May 12 2019, 11:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2019, 11:46 PM
Herald added a subscriber: kristina. · View Herald Transcript
bjope added a subscriber: bjope.May 13 2019, 12:01 AM

Adding "Z" makes sense.

If you're going to mess with the 64-bit builtins, it's probably worth adding a testcase that can be built with gcc to show that int64_t is actually correct. You should be able to write a C++ testcase using decltype (declare a variable of type "int64_t", then redeclare it as type "decltype(__builtin_bswap64(0))").

1 ↗(On Diff #199207)

I don't think this needs avr-registered-target; it doesn't actually invoke the backend or any optimization passes that depend on it.

Ka-Ka updated this revision to Diff 199385.May 14 2019, 2:44 AM
Ka-Ka marked an inline comment as done.

Added a new C++ testcase.
Removed the REQUIRES: avr-registered-target in the avr-builtins.c testcase.

efriedma added inline comments.May 14 2019, 2:36 PM
5 ↗(On Diff #199385)

You don't need quite so many targets on this list. There are essentially three interesting kinds of targets: targets where int is 16 bits (like avr), targets where int is 32 bits and long is 64 bits (like x86_64 Linux), and targets where int and long are both 32 bits (like i686 Linux).

You might need to pass -ffreestanding to avoid including /usr/include/stdint.h on non-Linux systems.

21 ↗(On Diff #199385)

If you write "extern uint16_t bswap16;", there won't be any error message when the types match.

Ka-Ka updated this revision to Diff 199559.May 15 2019, 12:01 AM
Ka-Ka marked 2 inline comments as done.

Updated according to review comments

This revision is now accepted and ready to land.May 15 2019, 11:46 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2019, 12:15 AM