[PATCH] D126857: [HLSL] Add WaveActiveCountBits as Langugage builtin function for HLSL

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/include/clang/Basic/Builtins.def:1697
+// HLSL
+LANGBUILTIN(WaveActiveCountBits, "Uib", "nc", HLSL_LANG)
+
----------------
python3kgae wrote:
> Anastasia wrote:
> > FYI we most of time try to add a builtin name using a reserved identifier which is not part of the language standard (mostly prefixed by `__`). Then we add regular function that just calls the clang builtins. This way provides more flexibility to the implementers. However you might not need this... in which case using original name avoids an extra call.
> Yes. For this one, it is without prefix to avoid extra call.
> I'm following things like enqueue_kernel in opencl.
> For other things support overload which has to make extra call, I'll add the prefix.
Ok, although `enqueue_kernel` was implemented as a builtin because it has a weird variadic prototype that can't be implemented using normal features of C/C++ languages. Hence it is a builtin with custom SemaChecking.


================
Comment at: clang/test/SemaHLSL/Wave.hlsl:7
+uint foo(bool b) {
+    return WaveActiveCountBits(b);
+}
----------------
python3kgae wrote:
> Anastasia wrote:
> > if you are planning to add a proper CodeGen test later then here it might be sufficient to have a `-verify` test and check that the builtin is accepted with the right arguments... you might want to add a test checking that a diagnostic is provided when the arguments are wrong... although this is not strictly necessary as we already test clang builtins quite extensively.
> Changed to verify test.
ok, then also `-ast-dump  -o -` can be removed from the Run line. Also you probably don't need `-finclude-default-header` here wither?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126857