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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

At a high-level: can you make sure that functional changes all have corresponding test coverage?



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


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11703-11704
   "the '%select{&|*|->}0' operator is unsupported in HLSL">;
-
+def err_hlsl_attribute_address_function_return_type : Error<
+  "function return type may not be qualified with an address space">;
 // Layout randomization diagnostics.
----------------
It looks like `err_opencl_return_value_with_address_space` could be modified to make this work. (The existing phrasing of 'return value' is strange and I'd be fine changing that to 'return type' instead.)


================
Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:59
+    Generic,  // ptr64
+    Generic,  // hlsl_gourpshared
 };
----------------



================
Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:83
+    Generic, // ptr64
+    Generic, // hlsl_gourpshared
 
----------------



================
Comment at: clang/lib/Basic/Targets/DirectX.h:44
+    0, // ptr64
+    3, // hlsl_gourpshared
 };
----------------



================
Comment at: clang/lib/Basic/Targets/NVPTX.h:46
+    0, // ptr64
+    0, // hlsl_gourpshared
 };
----------------



================
Comment at: clang/lib/Basic/Targets/SPIR.h:46
+    0, // ptr64
+    0, // hlsl_gourpshared
 };
----------------



================
Comment at: clang/lib/Basic/Targets/SPIR.h:76
+    0, // ptr64
+    0, // hlsl_gourpshared
 };
----------------



================
Comment at: clang/lib/Basic/Targets/TCE.h:53
     0, // ptr64
+    0, // hlsl_gourpshared
 };
----------------



================
Comment at: clang/lib/Basic/Targets/X86.h:47
+    272, // ptr64
+    0,   // hlsl_gourpshared
 };
----------------



================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1408
+        // Parse HLSL addr space attribute.
+        if (Tok.is(tok::kw_groupshared)) {
+          ParseHLSLQualifiers(DS.getAttributes());
----------------
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.


================
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();
+  }
----------------
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;` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135060