Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| clang/include/clang/Basic/BuiltinsX86.def | ||
|---|---|---|
| 1904 | 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. | |
| clang/include/clang/Basic/BuiltinsX86.def | ||
|---|---|---|
| 1904 | 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? | |
| clang/include/clang/Basic/BuiltinsX86.def | ||
|---|---|---|
| 1904 | Instead of new builtins, can we use builtin_clz, builtin_clzl, builin_ctz, builtin_ctzl? | |
| clang/include/clang/Basic/BuiltinsX86.def | ||
|---|---|---|
| 1904 | 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);
} | |
| clang/include/clang/Basic/BuiltinsX86.def | ||
|---|---|---|
| 1904 | 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. | |
| clang/lib/Headers/ia32intrin.h | ||
|---|---|---|
| 418 ↗ | (On Diff #249282) | This store also needs to be conditional on b being non-zero |
| clang/lib/Headers/ia32intrin.h | ||
|---|---|---|
| 456 ↗ | (On Diff #249546) | 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. |
| clang/lib/Headers/ia32intrin.h | ||
|---|---|---|
| 417 ↗ | (On Diff #249546) | Is extension needed? |
| clang/lib/Headers/ia32intrin.h | ||
|---|---|---|
| 417 ↗ | (On Diff #249546) | Nevermind it probably is needed. |
| clang/lib/Headers/ia32intrin.h | ||
|---|---|---|
| 456 ↗ | (On Diff #249546) | The variable shadowing problem when using statement expression is a known issue. https://patches-gcc.linaro.org/patch/17303/ In general, user variable names should not start with 2 underscores, so I think it's safe enough. |
| clang/lib/Headers/ia32intrin.h | ||
|---|---|---|
| 421 ↗ | (On Diff #249550) | Should (a) have a cast to (unsigned *)? |
| clang/lib/Headers/ia32intrin.h | ||
|---|---|---|
| 421 ↗ | (On Diff #249550) | 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. |
| clang/lib/Headers/ia32intrin.h | ||
|---|---|---|
| 421 ↗ | (On Diff #249550) | But if we implemented this as an inline function instead of a macro wouldn't the argument have been an unsigned *? |
| clang/lib/Headers/ia32intrin.h | ||
|---|---|---|
| 421 ↗ | (On Diff #249550) | 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? |
Reconsidered the advice of @rnk , we can use -fms-extensions to supported _BitScan* on linux.
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.