-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[Spirv][HLSL] Add OpAll lowering and float vec support #87952
Merged
farzonl
merged 10 commits into
llvm:main
from
farzonl:spirv-opall-lowering-and-float-vec-support
Apr 10, 2024
Merged
[Spirv][HLSL] Add OpAll lowering and float vec support #87952
farzonl
merged 10 commits into
llvm:main
from
farzonl:spirv-opall-lowering-and-float-vec-support
Apr 10, 2024
Conversation
This file contains 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
…stantNull to be configurable based on enviornment.
add opencl tests to make sure both paths are valid spirv
farzonl
requested review from
sudonatalie,
Keenuts,
VyacheslavLevytskyy and
michalpaszkowski
April 8, 2024 00:26
Keenuts
reviewed
Apr 8, 2024
…eCompositeOrNull into one update tests to look at Function Parameter
This was
linked to
issues
Apr 8, 2024
LGTM, thank you @farzonl |
VyacheslavLevytskyy
approved these changes
Apr 10, 2024
llvm-beanz
approved these changes
Apr 10, 2024
coopp
approved these changes
Apr 10, 2024
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.
Looks good to me. I am still getting used to seeing 'unsigned' being used without a type being specified.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The main point of this change was to add support for HLSL's all intrinsic.
In the process of doing that I found a few issues around creating an
OpConstantComposite
viabuildZerosVal
.First the current code didn't support floats so the process of adding
buildZerosValF
meant I needed afloat version of
getOrCreateIntConstVector
. After doing so I renamed both versions togetOrCreateConstVector
. That meant I needed to create a float type version ofgetOrCreateIntCompositeOrNull
. Luckily the type information was low for this function so was able to split it out into a helpwe and renamegetOrCreateIntCompositeOrNull
togetOrCreateCompositeOrNull
With the exception of type handling differences of the code and Null vs 0 Constant Op codes these functions should be identical.To handle scalar floats I could not use
buildConstantFP
like this PR did: 0a2aaab5aba46#diff-733a189c5a8c3211f3a04fd6e719952a3fa231eadd8a7f11e6ecf1e584d57411R1603 because that would create too many superfluous registers (that causes problems in the validator), I had to create a float version ofgetOrCreateConstInt
which I calledgetOrCreateConstFP
.similar problems with doing it like this: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp#L1540.
buildZerosValF
also has a use of a functiongetZeroFP
. This is because half, float, and double scalar values of 0 would collide inSPIRVDuplicatesTracker<Constant> CT
if you useAPFloat(0.0f)
.getORCreateConstFP
needed its own version ofgetOrCreateConstIntReg
which I calledgetOrCreateConstFloatReg
The one difference in this function isgetOrCreateConstFloatReg
returns a bit width so we don't have to callgetScalarOrVectorBitWidth
twice ie when it is used again ingetOrCreateConstFP
forOpConstantF
addNumImm
.getOrCreateConstFloatReg
needed anassignFloatTypeToVReg
helper which called agetOrCreateSPIRVFloatType
helper. There was no equivalent IntegerType::get for floats so I handled this with a switch statement on bit widths to get the right LLVM float type.Finally, there is the use of
bool ZeroAsNull = STI.isOpenCLEnv();
This is partly a cosmetic change. When Zeros are treated as nulls, we don't createOpConstantComposite
vectors which is something we do in the DXCs SPIRV backend. The DXC SPIRV backend also does not useOpConstantNull
. Finally, I needed a means to test the behavior of the OpConstantNull andOpConstantComposite
changes and this was one way I could do that via the same tests.