[PATCH] D122627: [HLSL] Fix MSFT Attribute parsing, add numthreads

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.


================
Comment at: clang/test/SemaHLSL/num_threads.hlsl:48
+#endif
+
+
----------------
aaron.ballman wrote:
> There are a few test cases that are missing here:
> 
> 1) Writing the attribute on the wrong subject.
> 2) Passing no args to the attribute, passing too many args to the attribute.
> 3) Function merging where the redeclaration has attributes that don't match. e.g.,
> ```
> [numthreads(1, 2, 3)] void func();
> [numthreads(4, 5, 6)] void func() {}
> ```
> 4) Should the user be able to pick the values via NTTP or other constant expressions? e.g.,
> ```
> template <int X, int Y, int Z>
> [numthreads(X, Y, Z)] void func();
> 
> constexpr int foo();
> [numthreads(foo(), 2, 3)] void other_func();
> ```
> 5) Can you write this attribute on a member function? Can the member function be virtual?
Great catches!

1-3 are definitely missing coverage. I'll add additional coverage. I also should add a test case where a value is a preprocessor macro.

For 4 & 5, the short answer is that HLSL doesn't support those situations, but I think there are two related diagnostics that I should be emitting:

1) Applying the numthreads attribute on a template function should warn (It won't work anyways, but better to issue the diagnostic)
2) Applying the numthreads attribute on a member function should 

The point by point explanation is:

4) Constexpr isn't supported at all, and user-defined templates are new to HLSL 2021, can't be used as entry functions. This _could_ be relaxed in the future, but we would need to force instantiation, which probably means some form of extern template support, which we don't have in the language yet.
5) Member functions also can't be used as entry functions, and HLSL doesn't support anything virtual.

I'll update the tests and diagnostics today.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122627