Remove assumption that templates are never UDTs#4752
Merged
llvm-beanz merged 6 commits intomicrosoft:mainfrom Dec 9, 2022
Merged
Remove assumption that templates are never UDTs#4752llvm-beanz merged 6 commits intomicrosoft:mainfrom
llvm-beanz merged 6 commits intomicrosoft:mainfrom
Conversation
pow2clk
reviewed
Nov 1, 2022
Collaborator
There was a problem hiding this comment.
I feel like we have a lot of these. To wit, we have a very similarly named function IsUserDefinedRecordType at around line 589 which uses a very different approach, but seems to have the same intent.
Can you explain the method used here and used previously? Maybe we can consolidate the UDT detection functions?
pow2clk
approved these changes
Dec 6, 2022
Collaborator
pow2clk
left a comment
pow2clk
left a comment
There was a problem hiding this comment.
Looks good! Go HLSL 2021! 🥳
llvm-beanz
added
the
for-release
tex3d
reviewed
Dec 6, 2022
tex3d
reviewed
Dec 6, 2022
tex3d
reviewed
Dec 6, 2022
tex3d
reviewed
Dec 6, 2022
tex3d
reviewed
Dec 6, 2022
tex3d
reviewed
Dec 6, 2022
tex3d
approved these changes
Dec 8, 2022
Contributor
tex3d
left a comment
tex3d
left a comment
There was a problem hiding this comment.
I don't love the name IsHLSLCopyableAnnotatableRecord, but don't think I have a perfect one in mind to replace it either.
There was an assumtion in the HLSL sema code that a template specialization could never be a UDT. This assumption is incorrect now. I've reworked the code so that we instead assume built-in types are marked as `implicit` (which they all should and seem to be). Correcting this in `IsHLSLNumericUserDefinedType` resulted in some breakge in raytracing code generation because we used that method to deterimine if structures could be payloads or attributes. That was an incorrect API usage because we do have some builtin types that are allowed. The change here does the following: * Introduce `IsUserDefinedType` which simply checks if a record type is implicit or a specialization of an implicit type. * Adjust `IsHLSLNumericUserDefinedType` to do what the name says. * Introduce `IsHLSLBuiltinRayAttributeStruct` to identify the builtin raytracing types. The new `IsHLSLBuiltinRayAttributeStruct` is hacky and uses the type names (as the old code did). We should in the future insert an internal attribute on the types that can be used to denote them so that we don't need to match string names. Resolves microsoft#4735
There are several methods across the codebase that detect UDTs using slightly different implementations. This change consolodates them.
IsHLSLBuiltinRayAttributeStruct should only return true for BuiltInTriangleIntersectionAttributes
../tools/clang/test/HLSLFileCheck/shader_targets/raytracing/builtin-ray- types-anyhit.hlsl ../tools/clang/test/HLSLFileCheck/shader_targets/raytracing/builtin-ray- types-callable.hlsl ../tools/clang/test/HLSLFileCheck/shader_targets/raytracing/builtin-ray- types-miss.hlsl
llvm-beanz
force-pushed
the
cbieneman/4735
branch
from
caed782 to
bc920b5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was an assumtion in the HLSL sema code that a template
specialization could never be a UDT. This assumption is incorrect now.
I've reworked the code so that we instead assume built-in types are
marked as
implicit(which they all should and seem to be).Correcting this in
IsHLSLNumericUserDefinedTyperesulted in somebreakge in raytracing code generation because we used that method to
deterimine if structures could be payloads or attributes. That was an
incorrect API usage because we do have some builtin types that are
allowed.
The change here does the following:
IsHLSLBuiltinRayAttributeStructwhich returns true for thebuiltin raytracing data types that behave like UDTs.
IsHLSLCopyableAnnotatableRecordreturns true foruser-defined trivially copyable structures and the builtin ray tracing
types.
IsHLSLNumericUserDefinedTypeto do what the name says.IsUserDefinedRecordTypeacrossthe project.
diagnostic cases missed by the existing tests.
The new
IsHLSLBuiltinRayAttributeStructis hacky and uses the typenames (as the old code did). We should in the future insert an internal
attribute on the types that can be used to denote them so that we don't
need to match string names.
Resolves #4735