This is an archive of the discontinued LLVM Phabricator instance.

[HLSL] Add shader attribute
ClosedPublic

Authored by python3kgae on Apr 16 2022, 10:58 PM.

Details

Summary

Shader attribute is for shader library identify entry functions.
Here's an example,

[shader("pixel")]
float ps_main() : SV_Target {

return 1;

}

When compile this shader to library target like -E lib_6_3, compiler needs to know ps_main is an entry function for pixel shader. Shader attribute is to offer the information.

A new attribute HLSLShader is added to support shader attribute. It has an EnumArgument which included all possible shader stages.

Diff Detail

Event Timeline

python3kgae created this revision.Apr 16 2022, 10:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2022, 10:58 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
python3kgae requested review of this revision.Apr 16 2022, 10:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2022, 10:58 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.
clang/include/clang/Basic/Attr.td
3978

anyhit vs closestHit in terms of capitalization seems a bit surprising to me -- is that intentional?

clang/include/clang/Basic/AttrDocs.td
6386–6388

Please add enough documentation that a user has a chance to use this feature properly (and it's fine to link to other documentation for more details if there's canonical documentation elsewhere that we don't want to duplicate).

clang/include/clang/Basic/DiagnosticSemaKinds.td
11598–11599 ↗(On Diff #423279)

Almost every other attribute warns on this situation and then ignores the attribute; why should this be a hard error? (A warning allows other implementations to support more enumeration values without breaking the user's code in Clang.)

clang/lib/Sema/SemaDeclAttr.cpp
6944–6947

Hmm, doesn't the common attribute handling logic already deal with this case? (It does for other attribute syntaxes, but we have almost no attributes using the Microsoft [] syntax, so I wouldn't be surprised if we need to add that logic.)

6961

Are you planning to do this TODO as part of this patch, or is this a FIXME someone needs to handle later?

clang/test/SemaHLSL/shader_attr.hlsl
20 ↗(On Diff #423279)

Formatting looks off here.

Also, the diagnostic text leaves a bit to be desired (this is more of a general problem with the HLSLEntry subject, so don't worry about it for your patch) -- this is a global function.

25 ↗(On Diff #423279)

Formatting looks off here as well. I'm guessing clang-format doesn't do anything good with Microsoft-style attributes.

python3kgae added inline comments.Apr 18 2022, 8:58 AM
clang/include/clang/Basic/Attr.td
3978

Good catch. It is not intentional.

clang/include/clang/Basic/AttrDocs.td
6386–6388

I'll fix this.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11598–11599 ↗(On Diff #423279)

I'll change it to warning.

clang/lib/Sema/SemaDeclAttr.cpp
6944–6947

I'll double check it I can remove this.

6961

It is a FIXME.

Add more detail to the doc.
Update the attribute name according to the doc.

Mostly looks good, just some nits with the documentation.

clang/include/clang/Basic/AttrDocs.td
6388
6389–6391

Fix doc.

clang/include/clang/Basic/AttrDocs.td
6389–6391

Thanks for the edit.
Updated.

This revision is now accepted and ready to land.Apr 19 2022, 9:34 AM

Thanks for the review.

bogner added a subscriber: bogner.Apr 20 2022, 1:06 PM
bogner added inline comments.
clang/include/clang/Basic/AttrDocs.td
6390

I'm guessing you meant to actually list the options here?

python3kgae added inline comments.Apr 20 2022, 1:44 PM
clang/include/clang/Basic/AttrDocs.td
6390

Good catch. Will fix it.

Fix doc again.

MaskRay added inline comments.Apr 20 2022, 10:01 PM
clang/test/SemaHLSL/shader_type_attr.hlsl
2

Add a file-level comment what this test is about

65

Add a space after CHECK:

Add comment for test.

MaskRay accepted this revision.Apr 20 2022, 11:02 PM
MaskRay added inline comments.
clang/include/clang/Basic/AttrDocs.td
6391

The one-space indentation seems unneeded

clang/lib/Sema/SemaDeclAttr.cpp
6956

Not performing a check is at most a TODO, not a FIXME, unless it may crash in a later pass without the check.

This revision was landed with ongoing or failed builds.Apr 20 2022, 11:46 PM
This revision was automatically updated to reflect the committed changes.

Committed.
Updated line number check for test.