KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
2.98k stars 823 forks source link

HLSL: reject-valid: can't return a boolean #2790

Open Ace17 opened 2 years ago

Ace17 commented 2 years ago

The following program is accepted by fxc.exe:

( http://shader-playground.timjones.io/acbd719135ab8a9cb080566f5fa69d6e )

struct TVertexOutput
{
    bool flag : DUMMY;
};

TVertexOutput vmain()
{
    TVertexOutput o = (TVertexOutput)0;
    o.flag = true;
    return o;
}

Howerver, it is rejected by glslang :

$ glslangValidator -V100 --spirv-val -S vert -D --target-env vulkan1.0 -e vmain -o test.spv test.vert.hlsl
test.vert.hlsl
error: SPIRV-Tools Validation Errors
error: If OpTypeBool is stored in conjunction with OpVariable, it can only be used with non-externally visible shader Storage Classes: Workgroup, CrossWorkgroup, Private, and Function
  %_entryPointOutput_flag = OpVariable %_ptr_Output_bool Output

( BTW, is there a way to get rid of the systematic printing of the input filename on stdout ? (i.e the first output line in the above command) )

Ace17 commented 2 years ago

Actually, the test case can be reduced to:

bool vmain() : DUMMY
{
    return true;
}
greg-lunarg commented 2 years ago

I think this is a problem with spirv-val, which is the validator used by glslangValidator.

Here is the SPIR-V generated by glslangValidator for the reduced test case:

               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %vmain "vmain" %_entryPointOutput
               OpExecutionMode %vmain OriginUpperLeft
               OpSource HLSL 500
               OpName %vmain "vmain"
               OpName %_entryPointOutput "@entryPointOutput"
               OpDecorate %_entryPointOutput Location 0
       %void = OpTypeVoid
          %3 = OpTypeFunction %void
       %bool = OpTypeBool
       %true = OpConstantTrue %bool
%_ptr_Output_bool = OpTypePointer Output %bool
%_entryPointOutput = OpVariable %_ptr_Output_bool Output
      %vmain = OpFunction %void None %3
          %5 = OpLabel
               OpStore %_entryPointOutput %true
               OpReturn
               OpFunctionEnd

The latest SPIR-V (1.5 rev 5) includes Input and Output in the list of allowed storage classes. I believe that as of the unified spec, these should be allowed no matter what version of Vulkan or SPIR-V you are targeting.

I am going to open an issue with spirv-val and close this issue. I you believe this is an error, please respond here and I will re-open.

greg-lunarg commented 2 years ago

It appears that while SPIR-V does allow boolean in user IO variables, Vulkan restricts them to numeric.

So the bug is in glslang and the solution is to implement boolean user IO variables as uint. One question to be answered: Should TRUE be 1 or -1? DirectX seems to use -1. Vulkan uses 1. Does it matter?

This will be more than a one line fix. I will try to get to somewhat soon, but if someone else wants to try to implement, please leave a note here.

greg-lunarg commented 2 years ago

BTW, is there a way to get rid of the systematic printing of the input filename on stdout ? (i.e the first output line in the above command

I don't know of one offhand, and scanning the usage and StandAlone.cpp did not reveal one.

DadSchoorse commented 2 years ago

( BTW, is there a way to get rid of the systematic printing of the input filename on stdout ? (i.e the first output line in the above command) )

If I understand you correctly, --quiet should do what you want.

Ace17 commented 2 years ago

As far as I understand from the --help strings, --quiet will silence all outputs, that might be useful.

The point I was trying to make here, is that systematically printing to stdout the name of the input file is just noise, and, unless there's something I'm missing here, we would probably be better off removing this feature completely from the code.

Does it make sense? Or have I been overlooking something here ?

DadSchoorse commented 2 years ago

The name is a bit confusing, but the only thing --quiet silences is https://github.com/KhronosGroup/glslang/blob/7f1d926a3a397f1d16bd1c36d2b35ba86891081c/StandAlone/StandAlone.cpp#L1329

Ace17 commented 2 years ago

Indeed. Thanks !