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

Invalid float4 to float assignment in codegen when using arrays of scalars (int[], float[]) in constant buffer #1455

Closed
Dredhog opened this issue Sep 1, 2020 · 3 comments · Fixed by #1456
Labels
bug Feature which should work in SPIRV-Cross does not for some reason. in progress Issue is being actively worked on

Comments

@Dredhog
Copy link
Contributor

Dredhog commented Sep 1, 2020

Hi,
I'm seeing a consistent code generation issue for this HLSL source snippet.

uniform float _BorderWidths[4];

float4 frag (float4 vertex : SV_POSITION) : SV_Target
{
	float2 result = float2(_BorderWidths[0], _BorderWidths[1]);

	if (vertex.x > 0)
		result.x = _BorderWidths[2];

	return float4(result, 0, 1);
}

The generated MSL doesn't add the .x when assigning a float array element to a vector variable's member (other targets beside MSL are affected as well e.g. glsl)

can reproduce the issue by compiling with either glslangValidator (SPIR-V generated locally with glslangValidator -D -e frag issueShader.txt -V -H --spirv-dis -S frag):

; SPIR-V
; Version: 1.0
; Generator: Khronos Glslang Reference Front End; 8
; Bound: 83
; Schema: 0
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %frag "frag" %vertex %_entryPointOutput
               OpExecutionMode %frag OriginUpperLeft
               OpSource HLSL 500
               OpName %frag "frag"
               OpName %_Global "$Global"
               OpMemberName %_Global 0 "_BorderWidths"
               OpName %_ ""
               OpName %vertex "vertex"
               OpName %_entryPointOutput "@entryPointOutput"
               OpDecorate %_arr_float_uint_4 ArrayStride 16
               OpMemberDecorate %_Global 0 Offset 0
               OpDecorate %_Global Block
               OpDecorate %_ DescriptorSet 0
               OpDecorate %_ Binding 0
               OpDecorate %vertex BuiltIn FragCoord
               OpDecorate %_entryPointOutput Location 0
       %void = OpTypeVoid
          %3 = OpTypeFunction %void
      %float = OpTypeFloat 32
    %v4float = OpTypeVector %float 4
    %v2float = OpTypeVector %float 2
       %uint = OpTypeInt 32 0
     %uint_4 = OpConstant %uint 4
%_arr_float_uint_4 = OpTypeArray %float %uint_4
    %_Global = OpTypeStruct %_arr_float_uint_4
%_ptr_Uniform__Global = OpTypePointer Uniform %_Global
          %_ = OpVariable %_ptr_Uniform__Global Uniform
        %int = OpTypeInt 32 1
      %int_0 = OpConstant %int 0
%_ptr_Uniform_float = OpTypePointer Uniform %float
      %int_1 = OpConstant %int 1
    %float_0 = OpConstant %float 0
       %bool = OpTypeBool
      %int_2 = OpConstant %int 2
    %float_1 = OpConstant %float 1
%_ptr_Input_v4float = OpTypePointer Input %v4float
     %vertex = OpVariable %_ptr_Input_v4float Input
%_ptr_Output_v4float = OpTypePointer Output %v4float
%_entryPointOutput = OpVariable %_ptr_Output_v4float Output
       %frag = OpFunction %void None %3
          %5 = OpLabel
         %54 = OpLoad %v4float %vertex
         %62 = OpAccessChain %_ptr_Uniform_float %_ %int_0 %int_0
         %63 = OpLoad %float %62
         %64 = OpAccessChain %_ptr_Uniform_float %_ %int_0 %int_1
         %65 = OpLoad %float %64
         %66 = OpCompositeConstruct %v2float %63 %65
         %68 = OpCompositeExtract %float %54 0
         %69 = OpFOrdGreaterThan %bool %68 %float_0
               OpSelectionMerge %70 None
               OpBranchConditional %69 %71 %70
         %71 = OpLabel
         %72 = OpAccessChain %_ptr_Uniform_float %_ %int_0 %int_2
         %73 = OpLoad %float %72
         %81 = OpCompositeInsert %v2float %73 %66 0
               OpBranch %70
         %70 = OpLabel
         %82 = OpPhi %v2float %66 %5 %81 %71
         %76 = OpCompositeExtract %float %82 0
         %77 = OpCompositeExtract %float %82 1
         %78 = OpCompositeConstruct %v4float %76 %77 %float_0 %float_1
               OpStore %_entryPointOutput %78
               OpReturn
               OpFunctionEnd

SPIRV-Cross generates the following MSL (taken from the shader-playground link above):

#include <simd/simd.h>

using namespace metal;

struct _Global
{
    float4 _BorderWidths[4];
};

struct frag_out
{
    float4 _entryPointOutput [[color(0)]];
};

fragment frag_out frag(constant _Global& _8 [[buffer(0)]], float4 gl_FragCoord [[position]])
{
    frag_out out = {};
    float2 _66 = float2(_8._BorderWidths[0].x, _8._BorderWidths[1].x);
    float2 _82;
    if (gl_FragCoord.x > 0.0)
    {
        float2 _81 = _66;
        _81.x = _8._BorderWidths[2];
        _82 = _81;
    }
    else
    {
        _82 = _66;
    }
    out._entryPointOutput = float4(_82, 0.0, 1.0);
    return out;
}

The issue is the _81.x = _8._BorderWidths[2]; line, as this is invalid MSL. Getting errors similar to the following from the metal compiler (snippet has been simplified since then so names don't match):

    _56.x = _Globals._BorderWidths[2];
        ^~~~~~~~~~~~~~~~~~~~~~~~~

DXC reproduction SPIR-V looks like this (dxc.exe -T ps_6_0 -E frag issueShader.txt -spirv ):

; SPIR-V
; Version: 1.0
; Generator: Google spiregg; 0
; Bound: 43
; Schema: 0
               OpCapability Shader
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %frag "frag" %gl_FragCoord %out_var_SV_Target
               OpExecutionMode %frag OriginUpperLeft
               OpSource HLSL 600
               OpName %type__Globals "type.$Globals"
               OpMemberName %type__Globals 0 "_BorderWidths"
               OpName %_Globals "$Globals"
               OpName %out_var_SV_Target "out.var.SV_Target"
               OpName %frag "frag"
               OpDecorate %gl_FragCoord BuiltIn FragCoord
               OpDecorate %out_var_SV_Target Location 0
               OpDecorate %_Globals DescriptorSet 0
               OpDecorate %_Globals Binding 0
               OpDecorate %_arr_float_uint_4 ArrayStride 16
               OpMemberDecorate %type__Globals 0 Offset 0
               OpDecorate %type__Globals Block
        %int = OpTypeInt 32 1
      %int_0 = OpConstant %int 0
      %int_1 = OpConstant %int 1
      %float = OpTypeFloat 32
    %float_0 = OpConstant %float 0
      %int_2 = OpConstant %int 2
       %uint = OpTypeInt 32 0
    %float_1 = OpConstant %float 1
     %uint_4 = OpConstant %uint 4
%_arr_float_uint_4 = OpTypeArray %float %uint_4
%type__Globals = OpTypeStruct %_arr_float_uint_4
%_ptr_Uniform_type__Globals = OpTypePointer Uniform %type__Globals
    %v4float = OpTypeVector %float 4
%_ptr_Input_v4float = OpTypePointer Input %v4float
%_ptr_Output_v4float = OpTypePointer Output %v4float
       %void = OpTypeVoid
         %21 = OpTypeFunction %void
    %v2float = OpTypeVector %float 2
%_ptr_Uniform_float = OpTypePointer Uniform %float
       %bool = OpTypeBool
   %_Globals = OpVariable %_ptr_Uniform_type__Globals Uniform
%gl_FragCoord = OpVariable %_ptr_Input_v4float Input
%out_var_SV_Target = OpVariable %_ptr_Output_v4float Output
       %frag = OpFunction %void None %21
         %25 = OpLabel
         %26 = OpLoad %v4float %gl_FragCoord
         %27 = OpAccessChain %_ptr_Uniform_float %_Globals %int_0 %int_0
         %28 = OpLoad %float %27
         %29 = OpAccessChain %_ptr_Uniform_float %_Globals %int_0 %int_1
         %30 = OpLoad %float %29
         %31 = OpCompositeConstruct %v2float %28 %30
         %32 = OpCompositeExtract %float %26 0
         %33 = OpFOrdGreaterThan %bool %32 %float_0
               OpSelectionMerge %34 None
               OpBranchConditional %33 %35 %34
         %35 = OpLabel
         %36 = OpAccessChain %_ptr_Uniform_float %_Globals %int_0 %int_2
         %37 = OpLoad %float %36
         %38 = OpCompositeInsert %v2float %37 %31 0
               OpBranch %34
         %34 = OpLabel
         %39 = OpPhi %v2float %31 %25 %38 %35
         %40 = OpCompositeExtract %float %39 0
         %41 = OpCompositeExtract %float %39 1
         %42 = OpCompositeConstruct %v4float %40 %41 %float_0 %float_1
               OpStore %out_var_SV_Target %42
               OpReturn
               OpFunctionEnd

Using DXC without optimisations however produces valid MSL when passed through SPIRV-Cross, this is source SPIR-V (generated with dxc.exe -T ps_6_0 -E frag issueShader.txt -spirv -O0):

; SPIR-V
; Version: 1.0
; Generator: Google spiregg; 0
; Bound: 59
; Schema: 0
               OpCapability Shader
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %frag "frag" %gl_FragCoord %out_var_SV_Target
               OpExecutionMode %frag OriginUpperLeft
               OpSource HLSL 600
               OpName %type__Globals "type.$Globals"
               OpMemberName %type__Globals 0 "_BorderWidths"
               OpName %_Globals "$Globals"
               OpName %out_var_SV_Target "out.var.SV_Target"
               OpName %frag "frag"
               OpName %param_var_vertex "param.var.vertex"
               OpName %src_frag "src.frag"
               OpName %vertex "vertex"
               OpName %bb_entry "bb.entry"
               OpName %result "result"
               OpName %if_true "if.true"
               OpName %if_merge "if.merge"
               OpDecorate %gl_FragCoord BuiltIn FragCoord
               OpDecorate %out_var_SV_Target Location 0
               OpDecorate %_Globals DescriptorSet 0
               OpDecorate %_Globals Binding 0
               OpDecorate %_arr_float_uint_4 ArrayStride 16
               OpMemberDecorate %type__Globals 0 Offset 0
               OpDecorate %type__Globals Block
        %int = OpTypeInt 32 1
      %int_0 = OpConstant %int 0
      %int_1 = OpConstant %int 1
      %float = OpTypeFloat 32
    %float_0 = OpConstant %float 0
      %int_2 = OpConstant %int 2
       %uint = OpTypeInt 32 0
     %uint_0 = OpConstant %uint 0
    %float_1 = OpConstant %float 1
     %uint_4 = OpConstant %uint 4
%_arr_float_uint_4 = OpTypeArray %float %uint_4
%type__Globals = OpTypeStruct %_arr_float_uint_4
%_ptr_Uniform_type__Globals = OpTypePointer Uniform %type__Globals
    %v4float = OpTypeVector %float 4
%_ptr_Input_v4float = OpTypePointer Input %v4float
%_ptr_Output_v4float = OpTypePointer Output %v4float
       %void = OpTypeVoid
         %22 = OpTypeFunction %void
%_ptr_Function_v4float = OpTypePointer Function %v4float
         %29 = OpTypeFunction %v4float %_ptr_Function_v4float
    %v2float = OpTypeVector %float 2
%_ptr_Function_v2float = OpTypePointer Function %v2float
%_ptr_Uniform__arr_float_uint_4 = OpTypePointer Uniform %_arr_float_uint_4
%_ptr_Uniform_float = OpTypePointer Uniform %float
%_ptr_Function_float = OpTypePointer Function %float
       %bool = OpTypeBool
   %_Globals = OpVariable %_ptr_Uniform_type__Globals Uniform
%gl_FragCoord = OpVariable %_ptr_Input_v4float Input
%out_var_SV_Target = OpVariable %_ptr_Output_v4float Output
       %frag = OpFunction %void None %22
         %23 = OpLabel
%param_var_vertex = OpVariable %_ptr_Function_v4float Function
         %26 = OpLoad %v4float %gl_FragCoord
               OpStore %param_var_vertex %26
         %27 = OpFunctionCall %v4float %src_frag %param_var_vertex
               OpStore %out_var_SV_Target %27
               OpReturn
               OpFunctionEnd
   %src_frag = OpFunction %v4float None %29
     %vertex = OpFunctionParameter %_ptr_Function_v4float
   %bb_entry = OpLabel
     %result = OpVariable %_ptr_Function_v2float Function
         %36 = OpAccessChain %_ptr_Uniform__arr_float_uint_4 %_Globals %int_0
         %38 = OpAccessChain %_ptr_Uniform_float %36 %int_0
         %39 = OpLoad %float %38
         %40 = OpAccessChain %_ptr_Uniform__arr_float_uint_4 %_Globals %int_0
         %41 = OpAccessChain %_ptr_Uniform_float %40 %int_1
         %42 = OpLoad %float %41
         %43 = OpCompositeConstruct %v2float %39 %42
               OpStore %result %43
         %45 = OpAccessChain %_ptr_Function_float %vertex %int_0
         %46 = OpLoad %float %45
         %48 = OpFOrdGreaterThan %bool %46 %float_0
               OpSelectionMerge %if_merge None
               OpBranchConditional %48 %if_true %if_merge
    %if_true = OpLabel
         %51 = OpAccessChain %_ptr_Uniform__arr_float_uint_4 %_Globals %int_0
         %52 = OpAccessChain %_ptr_Uniform_float %51 %int_2
         %53 = OpLoad %float %52
         %54 = OpAccessChain %_ptr_Function_float %result %int_0
               OpStore %54 %53
               OpBranch %if_merge
   %if_merge = OpLabel
         %55 = OpLoad %v2float %result
         %56 = OpCompositeExtract %float %55 0
         %57 = OpCompositeExtract %float %55 1
         %58 = OpCompositeConstruct %v4float %56 %57 %float_0 %float_1
               OpReturnValue %58
               OpFunctionEnd

SPIRV-Cross generates valid MSL in this case:

#pragma clang diagnostic ignored "-Wmissing-prototypes"

#include <metal_stdlib>
#include <simd/simd.h>

using namespace metal;

struct type_Globals
{
    float4 _BorderWidths[4];
};

struct frag_out
{
    float4 out_var_SV_Target [[color(0)]];
};

static inline __attribute__((always_inline))
float4 src_frag(thread const float4& vertex0, constant type_Globals& _Globals)
{
    float2 result = float2(_Globals._BorderWidths[0].x, _Globals._BorderWidths[1].x);
    if (vertex0.x > 0.0)
    {
        result.x = _Globals._BorderWidths[2].x;
    }
    return float4(result, 0.0, 1.0);
}

fragment frag_out frag(constant type_Globals& _Globals [[buffer(0)]], float4 gl_FragCoord [[position]])
{
    frag_out out = {};
    float4 param_var_vertex = gl_FragCoord;
    out.out_var_SV_Target = src_frag(param_var_vertex, _Globals);
    return out;
}

This SPIRV-Cross output is from shader playground but I'm seeing metal compilation errors with a few week old SPIRV-Cross build.

Also, note that the issue seems to disappear when the if conditional is removed or when the type of result is changed to a scalar.

@HansKristian-Work HansKristian-Work added the needs triage Needs to be reproduced before it can become a different issue type. label Sep 1, 2020
@HansKristian-Work
Copy link
Contributor

Thanks for the extensive repro case, this seems very weird, I'll have a look as soon as I can ...

@Dredhog
Copy link
Contributor Author

Dredhog commented Sep 2, 2020

Sounds good! Small note, I just reread the issue and found a small mismatch between the source and the SPIR-V, I've now updated the condition in the HLSL from if (!(vertex.x <= 0)) to if (vertex.x > 0) as that's what I actually used to generate the SPIR-V. The complexity of the actual condition shouldn't matter to reproduce it. Sorry for the confusion.

@HansKristian-Work HansKristian-Work added bug Feature which should work in SPIRV-Cross does not for some reason. in progress Issue is being actively worked on and removed needs triage Needs to be reproduced before it can become a different issue type. labels Sep 2, 2020
@HansKristian-Work
Copy link
Contributor

PR in flight, was just a trivial case of missing to_unpacked_expression in OpCompositeInsert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Feature which should work in SPIRV-Cross does not for some reason. in progress Issue is being actively worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants