[HLSL] Entry functions require param annotation

[beanz]

Summary

HLSL entry function parameters must have parameter annotations. This
allows appropriate intrinsic values to be populated into parameters
during code generation.

This does not handle entry function return values, which will be
handled in a subsequent commit because we don't currently support any
annotations that are valid for function returns.

[python3kgae]

clang/lib/Sema/SemaDecl.cpp

11875

When param type is struct, it is OK not to have HLSL semantic as long as all fields in the struct have HLSL semantic.

[beanz]

clang/lib/Sema/SemaDecl.cpp

11875

My intent was to handle structs in a subsequent patch. Since we only have one semantic implemented right now we can't make a valid test for structs unless it is a single element struct which is not a robust test case.

[aaron.ballman]

clang/include/clang/AST/Attr.h

193

Is this intended to be used only for parameters (that's how I read the summary for the patch)? If so, why is this not inheriting from InheritableParamAttr?

195–197

Formatting looks off here, you should run the patch through clang-format.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11635

Will users know how to fix the issue when they get this diagnostic? "missing annotation" sounds like "slap any old annotation on there, it'll be fine".

clang/lib/Sema/SemaDecl.cpp

11874

[beanz]

clang/include/clang/AST/Attr.h

193

Sadly these attributes are valid in places that aren’t parameters. For example, they can be applied on the members of a struct. When specifying an HLSL entry we require semantics for all scalar variables in the signature so that the we can match up the parameters from driver and user-provided values, but the parameters can be scalars or user defined structs. For example:

struct Pupper {
  int Legs : SomeAnnotation;
  int Ears : SomeOtherAnnotation;
}
...
void main(Pupper P) {...} // valid as an entry

We also allow these annotations (and require them) on return types for entry functions:

int main(...) : SomeAnnotation {...}

Where this all gets really special is that entry functions as passed to drivers have a void(void) signature, but entry functions with the source-specified signature can be called.

I'm trying to handle those cases in D131203 by generating the user-written function as is with C++ mangling, and generating a non-mangled void(void) wrapper that calls the underlying function after populating the annotation values. It is an incomplete implementation, but a starting point.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11635

I should probably make this closer to our existing diagnostic: "Semantic must be defined for all parameters of an entry function or patch constant function."

[aaron.ballman]

clang/include/clang/AST/Attr.h

193

Oh, thank you for the explanation!

Where this all gets really special is that entry functions as passed to drivers have a void(void) signature, but entry functions with the source-specified signature can be called.

Well that should be fun if you have code that cares about the address of the function, depending on which address you happen to get. But you don't allow pointers IIRC, so maybe you're "safe"?

clang/include/clang/Basic/DiagnosticSemaKinds.td

11635

That sounds more appropriately descriptive.

[beanz]

Updating based on PR feedback from @aaron.ballman.

I changed around the diagnostics so that the error is on the function decl with a note pointing at the parameter declaration.

[beanz]

clang/include/clang/AST/Attr.h

193

I _think_ taking the address of the function should consistently give the address of the mangled function since the AST will resolve using C++ mangling rules. I hope that would be the expected behavior even if we exposed pointers in the future since the void(void) entry should only ever be the hardware start point.

I'm sure I'm wrong about this and will regret this speculation in the future...

[aaron.ballman]

clang/include/clang/AST/Attr.h

193

Keep in mind that we have things like __builtin_frame_address() which exposes "this function" and "calling function" to the caller in ways that may be surprising. That said, if someone is using those kinds of tricks to inspect the identity of a function, I'm not certain how sympathetic I am to their plight in this case.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11635

I have no idea how close to the 80 col limit I got this, but we do mostly try to ensure diagnostics don't go too far over the 80 col limit (we're more lax in .td files because clang-format sometimes destroys the formatting rather than help it).

clang/lib/Sema/SemaDecl.cpp

11876

What about checking for the function return type?

[beanz]

Updates based on review feedback:

• Wraped diagnostic text in td
• Added FIXME for return type annotations

[aaron.ballman]

LGTM though I had a question about the test's RUN line.

clang/test/SemaHLSL/Semantics/missing_entry_annotation.hlsl

2

That's not needed, right?

[beanz]

clang/test/SemaHLSL/Semantics/missing_entry_annotation.hlsl

2

It shouldn't be but is because hlsl is missing from FrontendOptions::getInputKindForExtension. I'll get a patch up for that shortly.

[beanz]

clang/test/SemaHLSL/Semantics/missing_entry_annotation.hlsl

2

Since the change for this was pretty trivial, I pushed it in rG887bafb503c59c5ecef831c679a2b114ee6ef338

clang/include/clang/AST/Attr.h

 public:
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Attr *A) {
     return A->getKind() >= attr::FirstInheritableParamAttr &&
            A->getKind() <= attr::LastInheritableParamAttr;
   }
 };
 
+class HLSLAnnotationAttr : public InheritableAttr {
+protected:
+  HLSLAnnotationAttr(ASTContext &Context, const AttributeCommonInfo &CommonInfo,
+                     attr::Kind AK, bool IsLateParsed,
+                     bool InheritEvenIfAlreadyPresent)
+      : InheritableAttr(Context, CommonInfo, AK, IsLateParsed,
+                        InheritEvenIfAlreadyPresent) {}
+
+public:
+  // Implement isa/cast/dyncast/etc.
+  static bool classof(const Attr *A) {
+    return A->getKind() >= attr::FirstHLSLAnnotationAttr &&
+           A->getKind() <= attr::LastHLSLAnnotationAttr;
+  }
+};
+
 /// A parameter attribute which changes the argument-passing ABI rule
 /// for the parameter.
 class ParameterABIAttr : public InheritableParamAttr {
 protected:
   ParameterABIAttr(ASTContext &Context, const AttributeCommonInfo &CommonInfo,
                    attr::Kind AK, bool IsLateParsed,
                    bool InheritEvenIfAlreadyPresent)
       : InheritableParamAttr(Context, CommonInfo, AK, IsLateParsed,

clang/include/clang/Basic/Attr.td

 /// Some attributes, like calling conventions, can appear in either the
 /// declaration or the type position. These attributes are morally type
 /// attributes, but have historically been written on declarations.
 class DeclOrTypeAttr : InheritableAttr;
 
 /// A attribute is either a declaration attribute or a statement attribute.
 class DeclOrStmtAttr : InheritableAttr;
 
+/// An attribute class for HLSL Annotations.
+class HLSLAnnotationAttr : InheritableAttr;
+
 /// A target-specific attribute.  This class is meant to be used as a mixin
 /// with InheritableAttr or Attr depending on the attribute's needs.
 class TargetSpecificAttr<TargetSpec target> {
   TargetSpec Target = target;
   // Attributes are generally required to have unique spellings for their names
   // so that the parser can determine what kind of attribute it has parsed.
   // However, target-specific attributes are special in that the attribute only
   // "exists" for a given target. So two target-specific attributes can share

 def HLSLNumThreads: InheritableAttr {
   let Spellings = [Microsoft<"numthreads">];
   let Args = [IntArgument<"X">, IntArgument<"Y">, IntArgument<"Z">];
   let Subjects = SubjectList<[HLSLEntry]>;
   let LangOpts = [HLSL];
   let Documentation = [NumThreadsDocs];
 }
 
-def HLSLSV_GroupIndex: InheritableAttr {
+def HLSLSV_GroupIndex: HLSLAnnotationAttr {
   let Spellings = [HLSLSemantic<"SV_GroupIndex">];
   let Subjects = SubjectList<[ParmVar, GlobalVar]>;
   let LangOpts = [HLSL];
   let Documentation = [HLSLSV_GroupIndexDocs];
 }
 
 def HLSLShader : InheritableAttr {
   let Spellings = [Microsoft<"shader">];

clang/include/clang/Basic/DiagnosticSemaKinds.td

 
 // HLSL Diagnostics
 def err_hlsl_attr_unsupported_in_stage : Error<"attribute %0 is unsupported in %select{Pixel|Vertex|Geometry|Hull|Domain|Compute|Library|RayGeneration|Intersection|AnyHit|ClosestHit|Miss|Callable|Mesh|Amplification|Invalid}1 shaders, requires %2">;
 
 def err_hlsl_numthreads_argument_oor : Error<"argument '%select{X|Y|Z}0' to numthreads attribute cannot exceed %1">;
 def err_hlsl_numthreads_invalid : Error<"total number of threads cannot exceed %0">;
 def err_hlsl_missing_numthreads : Error<"missing numthreads attribute for %0 shader entry">;
 def err_hlsl_attribute_param_mismatch : Error<"%0 attribute parameters do not match the previous declaration">;
+def err_hlsl_missing_semantic_annotation : Error<"semantic annotations must be present for all parameters of an entry function or patch constant function">;
 
 def err_hlsl_pointers_unsupported : Error<
   "%select{pointers|references}0 are unsupported in HLSL">;
 
 def err_hlsl_operator_unsupported : Error<
   "the '%select{&|*|->}0' operator is unsupported in HLSL">;
 
 // Layout randomization diagnostics.
 def err_non_designated_init_used : Error<
   "a randomized struct can only be initialized with a designated initializer">;
 def err_cast_from_randomized_struct : Error<
   "casting from randomized structure pointer type %0 to %1">;
 } // end of sema component.

clang/lib/Sema/SemaDecl.cpp

void Sema::CheckHLSLEntryPoint(FunctionDecl *FD) {
   case llvm::Triple::EnvironmentType::Compute:
     if (!FD->hasAttr<HLSLNumThreadsAttr>()) {
       Diag(FD->getLocation(), diag::err_hlsl_missing_numthreads)
           << Triple.getEnvironmentName();
       FD->setInvalidDecl();
     }
     break;
   }
+
+  for (const auto *Param : FD->parameters()) {
+    if (!Param->hasAttr<HLSLAnnotationAttr>()) {
+      // FIXME: Handle struct parameters where annotations are on struct fields.
+      Diag(FD->getLocation(), diag::err_hlsl_missing_semantic_annotation);
+      Diag(Param->getLocation(), diag::note_previous_decl) << Param;
+      FD->setInvalidDecl();
+    }
+  }
 }
 
 bool Sema::CheckForConstantInitializer(Expr *Init, QualType DclT) {
   // FIXME: Need strict checking.  In C89, we need to check for
   // any assignment, increment, decrement, function-calls, or
   // commas outside of a sizeof.  In C99, it's the same list,
   // except that the aforementioned are allowed in unevaluated
   // expressions.  Everything else falls under the

clang/test/SemaHLSL/Semantics/missing_entry_annotation.hlsl

+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -fsyntax-only -hlsl-entry main -verify %s
+
+[numthreads(1,1, 1)]
+void main(int GI) { } // expected-error{{semantic annotations must be present for all parameters of an entry function or patch constant function}} expected-note{{'GI' declared here}}

clang/utils/TableGen/ClangAttrEmitter.cpp

 static const AttrClassDescriptor AttrClassDescriptors[] = {
   { "ATTR", "Attr" },
   { "TYPE_ATTR", "TypeAttr" },
   { "STMT_ATTR", "StmtAttr" },
   { "DECL_OR_STMT_ATTR", "DeclOrStmtAttr" },
   { "INHERITABLE_ATTR", "InheritableAttr" },
   { "DECL_OR_TYPE_ATTR", "DeclOrTypeAttr" },
   { "INHERITABLE_PARAM_ATTR", "InheritableParamAttr" },
-  { "PARAMETER_ABI_ATTR", "ParameterABIAttr" }
+  { "PARAMETER_ABI_ATTR", "ParameterABIAttr" },
+  { "HLSL_ANNOTATION_ATTR", "HLSLAnnotationAttr"}
 };
 
 static void emitDefaultDefine(raw_ostream &OS, StringRef name,
                               const char *superName) {
   OS << "#ifndef " << name << "\n";
   OS << "#define " << name << "(NAME) ";
   if (superName) OS << superName << "(NAME)";
   OS << "\n#endif\n\n";