[PATCH] D78663: [builtins] Add 32-bit shift builtins

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: compiler-rt/lib/builtins/int_ashl_impl.inc:13
+
+static __inline ashl_int_t __ashlXi3(ashl_int_t a, int b) {
+  const int bits_in_word = (int)(sizeof(ashl_int_t) * CHAR_BIT) / 2;
----------------
Use `inline`. We compile these files with -std=c11 (inline is a C99 keyword)


================
Comment at: compiler-rt/test/builtins/Unit/ashlsi3_test.c:3
+// REQUIRES: librt_has_ashlsi3
+//===-- ashlsi3_test.c - Test __ashlsi3 -----------------------------------===//
+//
----------------
aykevl wrote:
> MaskRay wrote:
> > Test files don't need file headers. There are plenty of examples in `clang/test/`
> All the other compiler-rt builtins tests (compiler-rt/test/builtins/Unit/*_test.c) do have a header. Should I deviate from that convention for new files?
> 
> One difference between these tests and the Clang tests is that these tests are in some sense full programs (with `main`) while Clang tests are usually just small incomplete source files, just testing whether the correct IR is generated.
> 
> (This file is a modified version of ashldi3_test.c, hence the style it follows).
I think you can deviate from them. They apparently deviate from the common practice in llvm.

`REQUIRES: ` should be place at the top.


================
Comment at: compiler-rt/test/builtins/Unit/ashlsi3_test.c:33
+int main() {
+  if (test__ashlsi3(0x01234567, 0, 0x1234567))
+    return 1;
----------------
It may be simpler organizing all tests in an array of arrays.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78663