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

MSL: Terminate function with return value using return if ending in unreachable. #2436

Merged
merged 1 commit into from
Jan 23, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#include <metal_stdlib>
#include <simd/simd.h>

using namespace metal;

struct buff_t
{
int m0[1024];
};

struct main0_out
{
float4 frag_clr [[color(0)]];
};

fragment main0_out main0(device buff_t& buff [[buffer(0)]], float4 gl_FragCoord [[position]])
{
main0_out out = {};
int2 frag_coord = int4(gl_FragCoord).xy;
int buff_idx = (frag_coord.y * 32) + frag_coord.x;
out.frag_clr = float4(0.0, 0.0, 1.0, 1.0);
buff.m0[buff_idx] = 1;
discard_fragment();
return out;
}

78 changes: 78 additions & 0 deletions shaders-msl/asm/frag/unreachable-return.msl23.spv14.asm.frag
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
; SPIR-V
; Version: 1.5
; Generator: Khronos Glslang Reference Front End; 11
; Bound: 46
; Schema: 0
OpCapability Shader
OpCapability DemoteToHelperInvocation
OpExtension "SPV_EXT_demote_to_helper_invocation"
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %main "main" %gl_FragCoord %frag_clr %buff
OpExecutionMode %main OriginUpperLeft
OpSource GLSL 450
OpName %main "main"
OpName %frag_coord "frag_coord"
OpName %gl_FragCoord "gl_FragCoord"
OpName %buff_idx "buff_idx"
OpName %frag_clr "frag_clr"
OpName %buff_t "buff_t"
OpMemberName %buff_t 0 "m0"
OpName %buff "buff"
OpDecorate %gl_FragCoord BuiltIn FragCoord
OpDecorate %frag_clr Location 0
OpDecorate %_arr_int_uint_1024 ArrayStride 4
OpDecorate %buff_t Block
OpMemberDecorate %buff_t 0 Offset 0
OpDecorate %buff Binding 0
OpDecorate %buff DescriptorSet 0
%void = OpTypeVoid
%3 = OpTypeFunction %void
%int = OpTypeInt 32 1
%v2int = OpTypeVector %int 2
%_ptr_Function_v2int = OpTypePointer Function %v2int
%float = OpTypeFloat 32
%v4float = OpTypeVector %float 4
%_ptr_Input_v4float = OpTypePointer Input %v4float
%gl_FragCoord = OpVariable %_ptr_Input_v4float Input
%v4int = OpTypeVector %int 4
%_ptr_Function_int = OpTypePointer Function %int
%uint = OpTypeInt 32 0
%uint_1 = OpConstant %uint 1
%int_32 = OpConstant %int 32
%uint_0 = OpConstant %uint 0
%_ptr_Output_v4float = OpTypePointer Output %v4float
%frag_clr = OpVariable %_ptr_Output_v4float Output
%float_0 = OpConstant %float 0
%float_1 = OpConstant %float 1
%34 = OpConstantComposite %v4float %float_0 %float_0 %float_1 %float_1
%uint_1024 = OpConstant %uint 1024
%_arr_int_uint_1024 = OpTypeArray %int %uint_1024
%buff_t = OpTypeStruct %_arr_int_uint_1024
%_ptr_StorageBuffer_buff_t = OpTypePointer StorageBuffer %buff_t
%buff = OpVariable %_ptr_StorageBuffer_buff_t StorageBuffer
%int_0 = OpConstant %int 0
%int_1 = OpConstant %int 1
%_ptr_StorageBuffer_int = OpTypePointer StorageBuffer %int
%main = OpFunction %void None %3
%5 = OpLabel
%frag_coord = OpVariable %_ptr_Function_v2int Function
%buff_idx = OpVariable %_ptr_Function_int Function
%14 = OpLoad %v4float %gl_FragCoord
%16 = OpConvertFToS %v4int %14
%17 = OpVectorShuffle %v2int %16 %16 0 1
OpStore %frag_coord %17
%22 = OpAccessChain %_ptr_Function_int %frag_coord %uint_1
%23 = OpLoad %int %22
%25 = OpIMul %int %23 %int_32
%27 = OpAccessChain %_ptr_Function_int %frag_coord %uint_0
%28 = OpLoad %int %27
%29 = OpIAdd %int %25 %28
OpStore %buff_idx %29
OpStore %frag_clr %34
%41 = OpLoad %int %buff_idx
%44 = OpAccessChain %_ptr_StorageBuffer_int %buff %int_0 %41
OpStore %44 %int_1
OpDemoteToHelperInvocation
OpUnreachable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This OpUnreachable is completely bogus fwiw. Having Unreachable post-dominate the entry block means that the entire shader is UB to execute, but ... whatever.

Copy link
Contributor Author

@squidbus squidbus Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I had the same suspicion and will probably have to visit the generation that results in this in my use case. But it seems to function elsewhere (I think just because discarding eliminates potential side effects?) and I figured also that, while the spec says that executing OpUnreachable is UB, there is no indication that it should fail to compile.

OpFunctionEnd
8 changes: 8 additions & 0 deletions spirv_glsl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17869,6 +17869,14 @@ void CompilerGLSL::emit_block_chain(SPIRBlock &block)

case SPIRBlock::Unreachable:
{
// If the entry point ends with unreachable and has a return value, insert a return
// statement to avoid potential compiler errors from non-void functions without a return value.
if (block.return_value)
{
statement("return ", to_unpacked_expression(block.return_value), ";");
break;
}

// Avoid emitting false fallthrough, which can happen for
// if (cond) break; else discard; inside a case label.
// Discard is not always implementable as a terminator.
Expand Down
8 changes: 5 additions & 3 deletions spirv_msl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4219,8 +4219,9 @@ uint32_t CompilerMSL::add_interface_block(StorageClass storage, bool patch)
// If the entry point should return the output struct, set the entry function
// to return the output interface struct, otherwise to return nothing.
// Watch out for the rare case where the terminator of the last entry point block is a
// Kill, instead of a Return. Based on SPIR-V's block-domination rules, we assume that
// any block that has a Kill will also have a terminating Return, except the last block.
// Kill or Unreachable, instead of a Return. Based on SPIR-V's block-domination rules,
// we assume that any block that has a Kill will also have a terminating Return, except
// the last block.
// Indicate the output var requires early initialization.
bool ep_should_return_output = !get_is_rasterization_disabled();
uint32_t rtn_id = ep_should_return_output ? ib_var_id : 0;
Expand All @@ -4230,7 +4231,8 @@ uint32_t CompilerMSL::add_interface_block(StorageClass storage, bool patch)
for (auto &blk_id : entry_func.blocks)
{
auto &blk = get<SPIRBlock>(blk_id);
if (blk.terminator == SPIRBlock::Return || (blk.terminator == SPIRBlock::Kill && blk_id == entry_func.blocks.back()))
auto last_blk_return = blk.terminator == SPIRBlock::Kill || blk.terminator == SPIRBlock::Unreachable;
if (blk.terminator == SPIRBlock::Return || (last_blk_return && blk_id == entry_func.blocks.back()))
blk.return_value = rtn_id;
}
vars_needing_early_declaration.push_back(ib_var_id);
Expand Down
Loading