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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
