Page MenuHomePhabricator

[HLSL] Entry functions require param annotation
ClosedPublic

Authored by beanz on Aug 10 2022, 3:16 PM.

Details

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.

Diff Detail

Event Timeline

beanz created this revision.Aug 10 2022, 3:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 3:16 PM
Herald added a subscriber: Anastasia. · View Herald Transcript
beanz requested review of this revision.Aug 10 2022, 3:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 3:16 PM
python3kgae added inline comments.Aug 10 2022, 5:40 PM
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 added inline comments.Aug 10 2022, 7:25 PM
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 added inline comments.Aug 11 2022, 6:22 AM
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 added inline comments.Aug 19 2022, 8:31 AM
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 added inline comments.Aug 19 2022, 11:25 AM
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 updated this revision to Diff 454676.Aug 22 2022, 7:38 PM

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 added inline comments.Aug 22 2022, 7:58 PM
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 added inline comments.Aug 23 2022, 6:22 AM
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 updated this revision to Diff 454892.Aug 23 2022, 10:30 AM

Updates based on review feedback:

  • Wraped diagnostic text in td
  • Added FIXME for return type annotations
aaron.ballman accepted this revision.Aug 23 2022, 11:40 AM

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

clang/test/SemaHLSL/Semantics/missing_entry_annotation.hlsl
1

That's not needed, right?

This revision is now accepted and ready to land.Aug 23 2022, 11:40 AM
beanz added inline comments.Aug 23 2022, 6:06 PM
clang/test/SemaHLSL/Semantics/missing_entry_annotation.hlsl
1

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

beanz added inline comments.Aug 23 2022, 6:53 PM
clang/test/SemaHLSL/Semantics/missing_entry_annotation.hlsl
1

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

This revision was landed with ongoing or failed builds.Aug 24 2022, 12:44 PM
This revision was automatically updated to reflect the committed changes.