[X86] Enable intrinsics _BitScan*

[skan]

Fix typo

[skan]

Format the patch due to the warning given by pre-merge check

[craig.topper]

clang/include/clang/Basic/BuiltinsX86.def

1904 ↗

(On Diff #248652)

The N specifier here is sort of MSVC mode specific. I need to think about this.

This also makes this available without including a header file which isn't good if it doesn't start with __builtin.

[skan]

clang/include/clang/Basic/BuiltinsX86.def

1904 ↗

(On Diff #248652)

I think we can define a new builtin __builtin_ia32_BitScanForward in BuiltinsX86.def. And when macro _MSC_VER is not defined, define _BitScanForward in ia32intrin.h by calling the __builtin_ia32_BitScanForward. Then we can avoid the N specifier and the name issue. Do you think it's appropriate?

[craig.topper]

clang/include/clang/Basic/BuiltinsX86.def

1904 ↗

(On Diff #248652)

Instead of new builtins, can we use builtin_clz, builtin_clzl, builin_ctz, builtin_ctzl?

[rnk]

clang/include/clang/Basic/BuiltinsX86.def

1904 ↗

(On Diff #248652)

Right, even though _BitScan* is in the implementers namespace, we want to be careful about adding builtins that don't start with __builtin_. Unless we set some dramatically new direction of making all the implementer's namespace MSVC builtins available everywhere, I don't see why we would do this when we already have equivalent builtins.

Is there some particular motivation as to why you want to make these available everywhere? -fms-extensions is kind of already available everywhere. This compiles on Linux with -fms-extensions:

extern unsigned char _BitScanReverse64(unsigned int *, unsigned long long);
unsigned char test_BitScanReverse64(unsigned int *index, unsigned long long mask) {
  return _BitScanReverse64(index, mask);
}

[skan]

clang/include/clang/Basic/BuiltinsX86.def

1904 ↗

(On Diff #248652)

My thought is that the page intel intrinsic guide says we can use the intrinsics _BitScan* as long as we include the header file immintrin.h, so we need support them on linux for user convenience even though similar intrinsics _bit_scan_* are available.

Since the purpose is to support intrinsics _BitScan* on linux rather than builtin _BitScan*, I think we can wrap the intrinsiscs with existing builtin as craig suggested.

[skan]

Enable _BitScan* as intrinsics rather than builtin on linux

[craig.topper]

clang/lib/Headers/ia32intrin.h

419

this evaluates (b) twice. That's bad if b is a function call with side effects

428

bsfq/bsrq is only defined on 64-bit targets.

[craig.topper]

clang/lib/Headers/ia32intrin.h

418

This store also needs to be conditional on b being non-zero

[skan]

Address review comments

[craig.topper]

clang/lib/Headers/ia32intrin.h

456

Variable name in macro needs to start with 2 underscores to avoid conflicts with user variable names. I need to think about whether that’s enough to avoid problems.

[craig.topper]

clang/lib/Headers/ia32intrin.h

417

Is extension needed?

[craig.topper]

clang/lib/Headers/ia32intrin.h

417

Nevermind it probably is needed.

[skan]

Make the variale name in macro start with 2 underscores

[skan]

clang/lib/Headers/ia32intrin.h

456

The variable shadowing problem when using statement expression is a known issue.

https://patches-gcc.linaro.org/patch/17303/
https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html

In general, user variable names should not start with 2 underscores, so I think it's safe enough.

[craig.topper]

clang/lib/Headers/ia32intrin.h

421

Should (a) have a cast to (unsigned *)?

[skan]

clang/lib/Headers/ia32intrin.h

421

No. Using a cast here may cause pointer aliasing problem.

https://stackoverflow.com/questions/98650/what-is-the-strict-aliasing-rule

If a is not unsigned *, letting the compiler do auto integer conversion for *(a) is the correct way, from my perspective.

[craig.topper]

clang/lib/Headers/ia32intrin.h

421

But if we implemented this as an inline function instead of a macro wouldn't the argument have been an unsigned *?

[skan]

clang/lib/Headers/ia32intrin.h

421

Yes, it would have been. But I don't known the motivation to use a cast (unsigned *) here, is there any case using cast here works correctly while not using cast here fails?

[skan]

Reconsidered the advice of @rnk , we can use -fms-extensions to supported _BitScan* on linux.

clang/lib/Headers/ia32intrin.h

 static __inline__ unsigned long long __attribute__((__always_inline__, __nodebug__))
 __rorq(unsigned long long __X, int __C) {
   return __builtin_rotateright64(__X, __C);
 }
 #endif /* __x86_64__ */
 
 #ifndef _MSC_VER
 /* These are already provided as builtins for MSVC. */
+#define _BitScanForward(a, b)                                                  \
+  __extension__({                                                              \
+    *(a) = (unsigned)__bsfd((int)(b));                                         \
+    (unsigned char)((b) != 0);                                                 \
+  })
+#define _BitScanReverse(a, b)                                                  \
+  __extension__({                                                              \
+    *(a) = (unsigned)__bsrd((int)(b));                                         \
+    (unsigned char)((b) != 0);                                                 \
+  })
+#define _BitScanForward64(a, b)                                                \
+  __extension__({                                                              \
+    *(a) = (unsigned)__bsfq((long long)(b));                                   \
+    (unsigned char)((b) != 0);                                                 \
+  })
+#define _BitScanReverse64(a, b)                                                \
+  __extension__({                                                              \
+    *(a) = (unsigned)__bsrq((long long)(b));                                   \
+    (unsigned char)((b) != 0);                                                 \
+  })
 /* Select the correct function based on the size of long. */
 #ifdef __LP64__
 #define _lrotl(a,b) __rolq((a), (b))
 #define _lrotr(a,b) __rorq((a), (b))
 #else
 #define _lrotl(a,b) __rold((a), (b))
 #define _lrotr(a,b) __rord((a), (b))
 #endif
 #define _rotl(a,b) __rold((a), (b))
 #define _rotr(a,b) __rord((a), (b))
 #endif // _MSC_VER
 
 /* These are not builtins so need to be provided in all modes. */
 #define _rotwl(a,b) __rolw((a), (b))
 #define _rotwr(a,b) __rorw((a), (b))
 
 #endif /* __IA32INTRIN_H */

clang/test/CodeGen/bitscan-builtins.c

 // RUN: %clang_cc1 -ffreestanding -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
 
 // PR33722
 // RUN: %clang_cc1 -ffreestanding -triple x86_64-unknown-unknown -fms-extensions -fms-compatibility-version=19.00 -emit-llvm -o - %s | FileCheck %s
 
 #include <x86intrin.h>
 
+unsigned char test_BitScanForward(unsigned *index, unsigned mask) {
+  return _BitScanForward(index, mask);
+  // CHECK: @test_BitScanForward
+  // CHECK: %{{.*}} = call i32 @llvm.cttz.i32(i32 %{{.*}}, i1 true)
+}
+
+unsigned char test_BitScanReverse(unsigned *index, unsigned mask) {
+  return _BitScanReverse(index, mask);
+  // CHECK: @test_BitScanReverse
+  // CHECK:  %[[call:.*]] = call i32 @llvm.ctlz.i32(i32 %{{.*}}, i1 true)
+  // CHECK:  %{{.*}} = sub nsw i32 31, %[[call]]
+}
+
+unsigned char test_BitScanForward64(unsigned *index, unsigned long long mask) {
+  return _BitScanForward64(index, mask);
+  // CHECK: @test_BitScanForward64
+  // CHECK: %{{.*}} = call i64 @llvm.cttz.i64(i64 %{{.*}}, i1 true)
+}
+
+unsigned char test_BitScanReverse64(unsigned *index, unsigned long long mask) {
+  return _BitScanReverse64(index, mask);
+  // CHECK: @test_BitScanReverse64
+  // CHECK:  %{{.*}} = call i64 @llvm.ctlz.i64(i64 %{{.*}}, i1 true)
+  // CHECK:  %{{.*}} = sub nsw i32 63, %{{.*}}
+}
+
 int test_bit_scan_forward(int a) {
   return _bit_scan_forward(a);
 // CHECK: @test_bit_scan_forward
 // CHECK: %[[call:.*]] = call i32 @llvm.cttz.i32(i32 %{{.*}}, i1 true)
 // CHECK: ret i32 %[[call]]
 }
 
 int test_bit_scan_reverse(int a) {