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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HLSL] cbuffer: Create host layout structs #122820
[HLSL] cbuffer: Create host layout structs #122820
Changes from 1 commit
71ddb5a
b88886b
07ec283
fc939db
0233994
75edd2b
bbdd56b
80fc426
a25ac0a
45ab868
aaf83f0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be multiple results if the name is defined in multiple accessible scopes (like current and parent scopes)? Or does it only return zero or one result from the specified scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only return zero or one result in the specified scope, which for the
HLSLBufferDecl
is the global scope. I will update theLookupNameKind
toLookupTagName
though to be more specific that the lookup is for a record/struct.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I don't see cases like this tested:
In what decl context will implicit host layout decls be created for each type? Will there be duplicates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean now. I have added a separate cbuffer AST test with namespaces and changed the implementation to always create the layout struct on the same context as the original struct so that it will be in the same namespace. The lookup is needs to scan just the immediate non-transparent decl context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't we want to catch more intangible types than just resources here? How do we make sure this is kept up-to-date for all intangible types?
On a related note, how do we ensure this code under
createHostLayoutStruct
always has matching/consistent logic to the code inrequiresImplicitBufferLayoutStructure
? Could there be a way to share the key logic components perhaps? For instance, there could be functions that return whether a leaf type is intangible (maybe this is justType::IsHLSLIntangibleType
), or is zero-sized, separate from the logic that recursively traverses a record type looking for these properties on field types. TheIsHLSLIntangibleType
and zero-sized detection functions are then used by both traversal paths to ensure consistent behavior.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, replied in wrong thread. Edited to move.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an assert at the top of the
createHostLayoutStruct
function that will make sure the logic is consistent:I have also moved the checking into a shared function
isInvalidConstantBufferLeafElementType
. Currently the only intangible types are resources classes, but I added a check for builtin resource type as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HLSL actually supports multiple
interface
bases (abstract bases), though we don't reliably implement these in DXC.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is accurate to say HLSL supports multiple bases. The
interface
support in DXC is HLSL 2015-only, we don't do anything sane with it in the language versions that DXC actually supports compiling for.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely true. We don't support use of interface instances (basically like pointers), but we do support interface definitions, inheritance from such, and overriding method implementations.
There is a bug in DXC when calling a method originally defined in an interface that is not inherited first in a multiple-inheritance scenario due to the way the MicrosoftCXXABI generates the code to access
this
for a method which we inherited by defaulting to that ABI in DXC.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so there can be multiple interfaces, but only one base struct, correct? And interfaces have only methods and no data members, so they can be safely ignored for the layout struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And Clang does not support interfaces yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have filed an issue and will add a FIXME to the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we might not be supporting interfaces in Clang, so I'll remove the FIXME.