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] Initialization of variables with its own value during declaration #6719

Closed
chaoticbob opened this issue Jun 24, 2024 · 1 comment
Closed
Labels
bug Bug, regression, crash spirv Work related to SPIR-V

Comments

@chaoticbob
Copy link

chaoticbob commented Jun 24, 2024

Description
Not sure if this is intentional or not but it looks horrifically scary to my C/C++ eyes. The shader below contains variables that use their own components for initialization during their declarations. DXC/SPIR-V seems to have no problem generating code for this - see SPIR-V below. DXC/DXIL generates this error:

error: validation errors
C:\local\Temp\21990660-6187-445f-ac50-5aef4415a2fc.hlsl:9:64: error: Instructions should not read uninitialized value.
note: at '%5 = fadd fast float %1, undef' in block '#0' of function 'main'.
C:\local\Temp\21990660-6187-445f-ac50-5aef4415a2fc.hlsl:9:53: error: Instructions should not read uninitialized value.
note: at '%8 = fadd fast float undef, %6' in block '#0' of function 'main'.
Validation failed.

My quick look at the SPIR-V shows that's initializing the component values before construction the composite variable. I don't know what happens if you run this code but the usage pattern just looks dangerous and possibly a fun way to create hard to find bugs. That said, I saw this in one of the DXC/SPIR-V tests where it looks somewhat intentional: https://github.com/microsoft/DirectXShaderCompiler/blob/main/tools/clang/test/CodeGenSPIRV/shader.debug.line.composite.hlsl#L64

Was curious if this is intentional? If it is, why?

Environment
Using DXC on https://shader-playground.timjones.io (Updated from trunk on 2024-04-29)

Shader

float4 main(float4 a : A) : SV_Target
{
    float2 v = float2(a.x, v.x);
    float2 v2 = float2(v2.y, v.x);
    
    float2x2 m = float2x2(v.x, m._m00, a.x, a.z);
    float2x2 m2 = float2x2(m._m01, v2.x, a.x, a.z);
    
    return float4(m._m00, m._m01, m2._m10, m2._m11) + float4(v + v2, 0, 0);
}

SPIR-V

; SPIR-V
; Version: 1.6
; Generator: Google spiregg; 0
; Bound: 81
; Schema: 0
               OpCapability Shader
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %main "main" %in_var_A %out_var_SV_Target
               OpExecutionMode %main OriginUpperLeft
               OpSource HLSL 610
               OpName %in_var_A "in.var.A"
               OpName %out_var_SV_Target "out.var.SV_Target"
               OpName %main "main"
               OpName %param_var_a "param.var.a"
               OpName %src_main "src.main"
               OpName %a "a"
               OpName %bb_entry "bb.entry"
               OpName %v "v"
               OpName %v2 "v2"
               OpName %m "m"
               OpName %m2 "m2"
               OpDecorate %in_var_A Location 0
               OpDecorate %out_var_SV_Target Location 0
        %int = OpTypeInt 32 1
      %int_0 = OpConstant %int 0
      %int_1 = OpConstant %int 1
      %int_2 = OpConstant %int 2
      %float = OpTypeFloat 32
    %float_0 = OpConstant %float 0
    %v4float = OpTypeVector %float 4
%_ptr_Input_v4float = OpTypePointer Input %v4float
%_ptr_Output_v4float = OpTypePointer Output %v4float
       %void = OpTypeVoid
         %14 = OpTypeFunction %void
%_ptr_Function_v4float = OpTypePointer Function %v4float
         %21 = OpTypeFunction %v4float %_ptr_Function_v4float
    %v2float = OpTypeVector %float 2
%_ptr_Function_v2float = OpTypePointer Function %v2float
%mat2v2float = OpTypeMatrix %v2float 2
%_ptr_Function_mat2v2float = OpTypePointer Function %mat2v2float
%_ptr_Function_float = OpTypePointer Function %float
   %in_var_A = OpVariable %_ptr_Input_v4float Input
%out_var_SV_Target = OpVariable %_ptr_Output_v4float Output
       %main = OpFunction %void None %14
         %15 = OpLabel
%param_var_a = OpVariable %_ptr_Function_v4float Function
         %18 = OpLoad %v4float %in_var_A
               OpStore %param_var_a %18
         %19 = OpFunctionCall %v4float %src_main %param_var_a
               OpStore %out_var_SV_Target %19
               OpReturn
               OpFunctionEnd
   %src_main = OpFunction %v4float None %21
          %a = OpFunctionParameter %_ptr_Function_v4float
   %bb_entry = OpLabel
          %v = OpVariable %_ptr_Function_v2float Function
         %v2 = OpVariable %_ptr_Function_v2float Function
          %m = OpVariable %_ptr_Function_mat2v2float Function
         %m2 = OpVariable %_ptr_Function_mat2v2float Function
         %33 = OpAccessChain %_ptr_Function_float %a %int_0
         %34 = OpLoad %float %33
         %35 = OpAccessChain %_ptr_Function_float %v %int_0
         %36 = OpLoad %float %35
         %37 = OpCompositeConstruct %v2float %34 %36
               OpStore %v %37
         %38 = OpAccessChain %_ptr_Function_float %v2 %int_1
         %39 = OpLoad %float %38
         %40 = OpAccessChain %_ptr_Function_float %v %int_0
         %41 = OpLoad %float %40
         %42 = OpCompositeConstruct %v2float %39 %41
               OpStore %v2 %42
         %43 = OpAccessChain %_ptr_Function_float %v %int_0
         %44 = OpLoad %float %43
         %45 = OpAccessChain %_ptr_Function_float %m %int_0 %int_0
         %46 = OpLoad %float %45
         %47 = OpAccessChain %_ptr_Function_float %a %int_0
         %48 = OpLoad %float %47
         %49 = OpAccessChain %_ptr_Function_float %a %int_2
         %50 = OpLoad %float %49
         %51 = OpCompositeConstruct %v2float %44 %46
         %52 = OpCompositeConstruct %v2float %48 %50
         %53 = OpCompositeConstruct %mat2v2float %51 %52
               OpStore %m %53
         %54 = OpAccessChain %_ptr_Function_float %m %int_0 %int_1
         %55 = OpLoad %float %54
         %56 = OpAccessChain %_ptr_Function_float %v2 %int_0
         %57 = OpLoad %float %56
         %58 = OpAccessChain %_ptr_Function_float %a %int_0
         %59 = OpLoad %float %58
         %60 = OpAccessChain %_ptr_Function_float %a %int_2
         %61 = OpLoad %float %60
         %62 = OpCompositeConstruct %v2float %55 %57
         %63 = OpCompositeConstruct %v2float %59 %61
         %64 = OpCompositeConstruct %mat2v2float %62 %63
               OpStore %m2 %64
         %65 = OpAccessChain %_ptr_Function_float %m %int_0 %int_0
         %66 = OpLoad %float %65
         %67 = OpAccessChain %_ptr_Function_float %m %int_0 %int_1
         %68 = OpLoad %float %67
         %69 = OpAccessChain %_ptr_Function_float %m2 %int_1 %int_0
         %70 = OpLoad %float %69
         %71 = OpAccessChain %_ptr_Function_float %m2 %int_1 %int_1
         %72 = OpLoad %float %71
         %73 = OpCompositeConstruct %v4float %66 %68 %70 %72
         %74 = OpLoad %v2float %v
         %75 = OpLoad %v2float %v2
         %76 = OpFAdd %v2float %74 %75
         %77 = OpCompositeExtract %float %76 0
         %78 = OpCompositeExtract %float %76 1
         %79 = OpCompositeConstruct %v4float %77 %78 %float_0 %float_0
         %80 = OpFAdd %v4float %73 %79
               OpReturnValue %80
               OpFunctionEnd

@chaoticbob chaoticbob added bug Bug, regression, crash needs-triage Awaiting triage spirv Work related to SPIR-V labels Jun 24, 2024
@Keenuts Keenuts removed the needs-triage Awaiting triage label Jul 9, 2024
@Keenuts
Copy link
Collaborator

Keenuts commented Jul 9, 2024

Thanks for the issue!

Interesting case. AFAIK the HLSL spec says nothing about that, but the C++ spec does defines the behaviors, and we can check what Clang/GCC is doing:

int c;
// standard: undefined.
// clang: initialized to 0.
// g++: undefined.

int b = b;
// standard: undefined.
// clang: initialized to 0, then `mov b, b`;
// g++: warn b is uninitialized, `mov b, b`

struct V { int x; int y; }
V v = { 0, v.x };
// standard: undefined.
// clang: move 0 into v.x, then v.x into v.y.
// g++: move 0 into v.x, then v.x into v.y

struct V2 {
    int x;
    int y;
    V2(int x, int y) : x(x), y(y) { }
}
V2 v2 = V2(0, v2.x);
// standard: undefined
// clang: warning, default init to 0, call V2 constructor with 0 & 0
// g++: warning, call constructor with 0 & undefined.

On the SPIR-V side, we do what G++ does: we allow the statement, but we move uninitialized values.
This behavior seems fair to me: you are doing something undefined, we respect that.
Once optimized, the bad values should end-up with an OpUndef.

What could be nice however is to warn if an OpUndef ends up being stored into either a resource, or returned by a shader (as I don't see a case where that's desirable). But correctly detecting those would require a complex static analysis tooling we don't have today.

I suppose I can close this as "working as intended", but feel free to re-open if you disagree.

@Keenuts Keenuts closed this as not planned Won't fix, can't repro, duplicate, stale Jul 9, 2024
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

No branches or pull requests

2 participants