[PATCH] D124752: [HLSL] clang codeGen for HLSLShaderAttr.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2606
+  if (getLangOpts().HLSL && TargetDecl) {
+    if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(TargetDecl))
+      getHLSLRuntime().addHLSLFunctionAttributes(FuncAttrs, RetAttrs, FD);
----------------
I think `TargetDecl` can't be nullptr at this point due to the above check? So perhaps using `dyn_cast` is better here?


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:55
+void CGHLSLRuntime::addHLSLFunctionAttributes(llvm::AttrBuilder &FuncAttrs,
+                                              llvm::AttrBuilder &RetAttrs,
+                                              const FunctionDecl *FD) {
----------------
This parameter seems to be unused?


================
Comment at: clang/test/CodeGenHLSL/shader_type_attr.hlsl:3
+
+// Make sure not mangle entry.
+// CHECK:define void @foo()
----------------
python3kgae wrote:
> Anastasia wrote:
> > I struggle understand the difference in this test? Can it be merged with the above?
> The difference between these 2 tests is entry.hlsl test for entry which does not have shader attribute on foo while shader_type_attr.hlsl test for entry in a library which has shader attribute (the [shader("compute")]).
Ok, I see although in this patch you only add support for generating the attribute in IR so the logic for handling `-E` or input attribute is I assume elsewhere and it is tested separately?

But the tests are small so no big concerns to keep both either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124752