[SPIR-V] Fix crash with GetAttributeAtVertex#6018
Closed
Keenuts wants to merge 1 commit into
Closed
Conversation
The code assumed all the types we handled had an AST type. But when the struct is created in the SPIR-V backend, there are no AST type left to represent it. In such case, the lowering step should handle the interpolation attributes, so I think we shouldn't have to do anything. Signed-off-by: Nathan Gauër <brioche@google.com>
Keenuts
force-pushed
the
fix-getattribute
branch
from
54dc4e3 to
df553c6
Compare
Contributor
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
s-perron
requested changes
Nov 15, 2023
Collaborator
s-perron
left a comment
s-perron
left a comment
There was a problem hiding this comment.
It is hard for me to know if it is correct. I want to see that the expansion of GetAttributeAtVertex is still correct. I also do not understand why the type does not have to be expanded. Please update the comment with more details. Where does it get expanded?
| bool PervertexInputVisitor::expandNointerpVarAndParam( | ||
| SpirvInstruction *spvInst) { | ||
| // If the spirv type has no AST type (hybrid struct for ex), no need to expand | ||
| // it. |
Collaborator
There was a problem hiding this comment.
Can you explain why there is no need to expand it?
|
|
||
| float colorAtV0 = GetAttributeAtVertex(input.color, 0)[cmp]; | ||
| float colorAtV1 = GetAttributeAtVertex(input.color, 1)[cmp]; | ||
| float colorAtV2 = GetAttributeAtVertex(input.color, 2)[cmp]; |
Collaborator
There was a problem hiding this comment.
Could you add checks to make sure the correct values are extracted from the input?
Contributor
|
I'll help to take a look. |
cassiebeckley
approved these changes
Nov 21, 2023
Contributor
cassiebeckley
left a comment
cassiebeckley
left a comment
There was a problem hiding this comment.
LGTM once Steven's comments are addressed
sudonatalie
added a commit
that referenced
this pull request
Mar 5, 2024
…ture inputs. (#6041) **Fix for** Fix issue under: #2955 For structure type, when `nointerpolation` is decorated on a structure input, this flag should be broadcast to its members. PR (#6018) also helps to resolve an issue when SPIRV backend generated variable has no AST type. **Test** Please see same case under: #6018 Besides, example as below should be invalid as the first parameter of function `compute` should has only one spirv-type. If its parameter type declaration has been expanded to an array implicitly, it should not accept other interpolated inputs as its input parameter. ``` struct S { float4 a : COLOR; }; float compute(float4 a) { return GetAttributeAtVertex(a, 2)[0]; } float4 main(nointerpolation S s, float4 b : COLOR2) : SV_TARGET //float4 main(nointerpolation S s) : SV_TARGET { return float4(0, 0, compute(b), compute(s.a)); //return float4(0, 0, 0, compute(s.a)); } ``` I added an error report point in this commit and gets following reports: ``` 1.hlsl:12:31: error: Current function could only use noninterpolated variable as input. return float4(0, 0, compute(b), compute(s.a)); ^ fatal error: generated SPIR-V is invalid: OpFunctionCall Argument <id> '37[%param_var_a_0]'s type does not match Function <id> '24[%_ptr_Function_v4float]'s parameter type. %45 = OpFunctionCall %float %compute %param_var_a_0 note: please file a bug report on https://github.com/Microsoft/DirectXShaderCompiler/issues with source code if possible ``` Any idea to let the error only reported before spirv validator? **Ref**: #2955 #6018 --------- Co-authored-by: Zhou, Shaochi(AMD) <shaozhou@amd.com> Co-authored-by: Natalie Chouinard <sudonatalie@google.com>
Collaborator
Author
|
@sudonatalie closing this PR: the bug this PR fixes is already fixed upstream thanks to the other PR. |
Collaborator
Author
|
Was wrong, the repro case I had is fixed, but not the issue case. |
Collaborator
Author
|
Replaced by #6459 |
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.
The code assumed all the types we handled had an AST type. But when the struct is created in the SPIR-V backend, there are no AST type left to represent it.
In such case, the lowering step should handle the interpolation attributes, so I think we shouldn't have to do anything.
Fixes #2955 again.