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

[Xiang Li via Phabricator via cfe-commits]

python3kgae added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:4045
+def HLSLGroupSharedAddressSpace : TypeAttr {
+  let Spellings = [Keyword<"groupshared">, Clang<"groupshared">];
+  let Documentation = [HLSLGroupSharedAddressSpaceDocs];
----------------
aaron.ballman wrote:
> So this is being exposed as both a keyword and an attribute? Why? (No tests are using it as an attribute.)
It should be only keyword. I misunderstood what the Clang<> means here :(


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1408
+        // Parse HLSL addr space attribute.
+        if (Tok.is(tok::kw_groupshared)) {
+          ParseHLSLQualifiers(DS.getAttributes());
----------------
aaron.ballman wrote:
> Do you expect to add more address spaces for HLSL? Do we want a helper function here for `isHLSLQualifier()` instead?
> 
> Also, there's no test coverage for this change.
Added isHLSLQualifier.
Maybe not other address space in a short term but will need other qualifiers for sure. :)

HLSL doesn't support lambda at this moment.
Should I just remove this and add it back when lambda is supported?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:14927-14931
+  if (ResultType.getAddressSpace() != LangAS::Default && getLangOpts().HLSL) {
+    Diag(FD->getLocation(),
+         diag::err_hlsl_attribute_address_function_return_type);
+    FD->setInvalidDecl();
+  }
----------------
aaron.ballman wrote:
> Why is this being done from `ActOnStartOfFunctionDef()` instead of `CheckFunctionReturnType()`?
> 
> We're missing test coverage for specifying this on a function declaration, a friend function declaration, or a member function (declaration or definition).
> 
> What about non-function situations like conversion operators? Can you write `operator groupshared int() const;` ?
Moved to ActOnFunctionDeclarator following err_opencl_return_value_with_address_space.
Also added tests.
operator overload is not supported in current version of hlsl though.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135060