Skip to content
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

[SPIR-V] OpConstantNull Result Type <id> '12[%type_2d_image]' cannot have a null value #6653

Closed
webez opened this issue May 27, 2024 · 3 comments · Fixed by #6686
Closed
Assignees
Labels
bug Bug, regression, crash spirv Work related to SPIR-V

Comments

@webez
Copy link

webez commented May 27, 2024

Description
Invalid spirv generated when calling a function that returns a texture depending on a condition

Steps to Reproduce

Shader Playground

Actual Behavior
fatal error: generated SPIR-V is invalid: OpConstantNull Result Type '12[%type_2d_image]' cannot have a null value.
%13 = OpConstantNull %type_2d_image

Environment
DXC Trunk, no optimization

@webez webez added bug Bug, regression, crash needs-triage Awaiting triage spirv Work related to SPIR-V labels May 27, 2024
@Keenuts Keenuts removed the needs-triage Awaiting triage label May 29, 2024
@Keenuts
Copy link
Collaborator

Keenuts commented May 29, 2024

Hi!

Thanks for the report & nice catch!
For some reasons when the return is not defined for a branch (here the compiler don't know both branches of the if/else returned), we generate an OpConstantNull instead of an OpUndef.
I'll fix that.

@Keenuts Keenuts self-assigned this May 29, 2024
@s-perron
Copy link
Collaborator

@Keenuts See if you can find the reason in the history. I prefer using OpConstantNull when possible because if that code is triggered, it will give consistent results. That would be easier to debug. For types where OpConstantNull is not allowed, using undef is a good.

@Keenuts
Copy link
Collaborator

Keenuts commented May 29, 2024

Original addition was #809
There don't seem to be any mention of why this was done.

As for using OpConstantNull instead: I would be against: the OpUndef has the advantage to be clear: it is not a desired value. Meaning we can do static analysis on the SPIR-V, and possibly find cases where there is a bug. If we use OpConstantNull, then we cannot differentiate actual null from bugs.

Keenuts added a commit to Keenuts/DirectXShaderCompiler that referenced this issue Jun 11, 2024
Before this change, OpConstantNull was emitted when an undef value was
required.
This causes an issue for some types which cannot have the OpConstantNull
value.

In addition, it mixed well-defined values with undefined values, which
prevents any kind of optimization/analysis later on.

Fixes microsoft#6653

Signed-off-by: Nathan Gauër <brioche@google.com>
Keenuts added a commit to Keenuts/DirectXShaderCompiler that referenced this issue Jun 12, 2024
Before this change, OpConstantNull was emitted when an undef value was
required.
This causes an issue for some types which cannot have the OpConstantNull
value.

In addition, it mixed well-defined values with undefined values, which
prevents any kind of optimization/analysis later on.

Fixes microsoft#6653

Signed-off-by: Nathan Gauër <brioche@google.com>
Keenuts added a commit that referenced this issue Jun 13, 2024
Before this change, OpConstantNull was emitted when an undef value was
required.
This causes an issue for some types which cannot have the OpConstantNull
value.

In addition, it mixed well-defined values with undefined values, which
prevents any kind of optimization/analysis later on.

Fixes #6653

---------

Signed-off-by: Nathan Gauër <brioche@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug, regression, crash spirv Work related to SPIR-V
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants