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.
[HLSL] Make memory representation of boolean vectors in HLSL, vectors of i32. Add support for boolean swizzling. #123977
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] Make memory representation of boolean vectors in HLSL, vectors of i32. Add support for boolean swizzling. #123977
Changes from all commits
50f8b16
b231f4e
dd7e459
6734eed
d9a4777
e0638d1
2e534a5
793541d
1fe0951
505b17b
0e6da2b
fbaf536
3d22ed0
7a0ccdd
d3904c4
e947d2c
5b364cb
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.
This might be a dumb suggestion, but is the a way to just check if the mem reprsentation is i32 or i1? HLSL is probably the only language mode that needs this distinction but it feel like this shouldn't have a lang opt toggle based on the function name.
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.
The code to state that a bool vector should be a vector of i32s isn't accessible here.
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 think it would make for slightly better abstraction to add a
hasPackedBoolVectors()
accessor toLangOptions
that returns!HLSL
, similar to howallowArrayReturnTypes()
works.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.
you replaced a bunch of
isExtVectorBoolType()
withisPackedVectorBoolType
. We are only doing HLSL modifications on theisExtVectorBoolType()
. ButisExtVectorBoolType
doesn't mean the vector is not packed. Is the Zero extend and truncation to get them into a form that they will unpack?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.
My intention was to replace calls to 'isExtVectorBoolType' with 'isPackedVectorBoolType' anywhere we want an hlsl boolean vector to follow the normal handling path for vectors; Hopefully reviews will verify I got this right.
Here we can't follow the normal vector path because it returns the value unchanged, and we need to convert a vec of i1s to a vec of i32s, which is why we zero extend here. The normally "boolean vector packing" does something different.
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.
Shouldn't this have a check for bitsize to avoid a no-op trunc, like we do elsewhere in this patch?
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.
Probably better to phrase this comment in terms of packed/non-packed boolean vectors given the abstraction in LangOpts
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.
Same here
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 could use the
hasPackedBoolVectors
accessor I suggested elsewhere.