[PATCH] D135060: [HLSL] Add groupshare address space.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10299
   Error<"attribute %0 can only be applied to an OpenCL kernel function">;
-def err_opencl_return_value_with_address_space : Error<
-  "return value cannot be qualified with address space">;
+def err_opencl_hlsl_return_value_with_address_space : Error<
+  "return type cannot be qualified with address space">;
----------------
I think we can drop the opencl and hlsl from the name at this point.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11710
   "the '%select{&|*|->}0' operator is unsupported in HLSL">;
-
 // Layout randomization diagnostics.
----------------
Looks like this change can be reverted out.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:4344
+    case tok::kw_groupshared:
+      ParseHLSLQualifiers(DS.getAttributes());
+      break;
----------------
It took me a while to convince myself that this was correct -- the token is consumed at the end of the `while` loop.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5841
+    case tok::kw_groupshared:
+      ParseHLSLQualifiers(DS.getAttributes());
+      break;
----------------
Same thing here -- the end of the `while` loop is what consumes the token, so this is also correct.


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1410
+          ParseHLSLQualifiers(DS.getAttributes());
+          ConsumeToken();
+        }
----------------
And this consume is correct because the parse function doesn't consume any tokens.


================
Comment at: clang/test/SemaHLSL/group_shared.hlsl:57
+ // expected-error at +1 {{no matching function for call to 'tfoo'}}
+ GSF gs2 = tfoo<GSF>(gs);
----------------
Some other Sema test cases that would be useful:
```
// Do we reject it on a return type in a function pointer?
groupshared void (*fp)();
// What about on parameters in a function pointer?
void (*fp2)(groupshared float);
// Trailing return types?
auto func() -> groupshared void;
// What about in a novel qualifier order?
void groupshared f();

struct S {
  // Do we reject it as a function qualifier on a member function?
  void f() groupshared;
};

// Does it impact size or alignment?
static_assert(sizeof(float) == sizeof(groupshared float));
static_assert(alignof(double) == alignof(groupshared double));

// Does it impact type identity for templates?
template <typename Ty>
struct S {
  static constexpr bool value = false;
};

template <>
struct S<groupshared float> {
  static constexpr bool value = true;
};
static_assert(!S<float>::value);
static_assert(S<groupshared float>::value);

// Can you overload based on the qualifier?
void func(float f) {}
void func(groupshared float f) {} // Error on redefinition?
```
(Note, I'm taking guesses on some of these test cases; if I've guessed wrong, feel free to correct me!)

I'd also expect tests to show up in ParserHLSL testing the parsing behavior:
```
// Do we accept novel qualifier orders?
extern groupshared float f;
extern float groupshared f; // Ok, redeclaration?

// Can we parse it on a lambda? Note, be sure to add more lambda tests with 
// more complex parsing situations to make sure you're happy with the parsing
// behavior.
[]() groupshared {};

// Can we still parse attributes written on the type?
float groupshared [[]] i = 12;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135060