skan marked an inline comment as done.
skan added inline comments.
================
Comment at: clang/include/clang/Basic/BuiltinsX86.def:1904
+// BITSCAN
+TARGET_BUILTIN(_BitScanForward, "UcUNi*UNi", "n", "")
+TARGET_BUILTIN(_BitScanReverse, "UcUNi*UNi", "n", "")
----------------
rnk wrote:
> craig.topper wrote:
> > skan wrote:
> > > craig.topper wrote:
> > > > 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.
> > > 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?
> > Instead of new builtins, can we use __builtin_clz, __builtin_clzl,
> > __builin_ctz, __builtin_ctzl?
> 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);
> }
> ```
My thought is that the page [[
https://software.intel.com/sites/landingpage/IntrinsicsGuide/#expand=408,410,404,460,404,405,408&text=BitScan
| 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75723/new/
https://reviews.llvm.org/D75723
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
