Problem with DXIL signing and switch case/enum use

[LautrecOfCarim]

Hi! I'm having a bit of trouble with emitted shader code which is then passed to DXC and I think this case qualifies as a bug:

enum class QualityT { Low, Medium, High, };

struct MyStruct
{
     float4 m_color;
     uint4  m_shaderVariantKey[1];
};
ConstantBuffer<MyStruct> MyCB : register(b0);

::QualityT __GetQuality()
{
    uint shaderKey = (MyCB.m_shaderVariantKey[0].w >> 0) & 3;
    return (::QualityT) shaderKey;
}

float4 MainPS() :SV_Target0
{
    static const int IntOption = 4;
switch ( __GetQuality() )
{
    case QualityT::Low:    return MyCB.m_color * float4 (IntOption, 0, 0, 0) ;
    case QualityT::Medium: return MyCB.m_color * float4 (0, IntOption, 0, 0) ;
    case QualityT::High:   return MyCB.m_color * float4 (0, 0, IntOption, 0) ;
} 
}

The code fails validation:

warning: DXIL.dll not found.  Resulting DXIL will not be signed for use in release environments.

error: validation errors
at 0x24f9e5b2a10 inside block #0 of function MainPS Instructions must be of an allowed type

Validation failed.

Shader playground link: http://shader-playground.timjones.io/5a194437e2936994ce22465b0ffdea4b

The issue doesn't end here. Commenting out any one of the three switch cases results in a valid DXIL code (but it shouldn't, because there is no default case). Adding a new enumerator results in failed validation + correct error message (enum class QualityT { Low, Medium, High, Fake };).

Finally, adding a default case fixed the signing validation problem, although there are no enumerations that fall in the default case: http://shader-playground.timjones.io/43eb8687ee339c98c7d52bebaf766d56

Thanks, Alex

[tristanlabelle]

I think there are multiple issues here:

• The validator errors are too oscure. What actually happens is that Clang generates an "unreachable" instruction for the default code path, and this instruction is invalid in DXIL.
• Clang seems to optimize a switch case with only two cases to an if-then-else during initial codegen, which is why there is no "unreachable" instruction in this case. Since the unreachable case is undefined behavior, it is free to do this.
• The "not all cases handled" warning looks like it's not fully reliable
• We should treat "Control flow can reach the end of a non-void function" as an error during semantic analysis rather than waiting for the validator to catch it.

Your last remark is not a bug however. Nothing garantees that an enum-typed value will be equal to any of the enumerants' values, so it is legit and desirable to have a default case.

[LautrecOfCarim]

Thanks for the detailed clarification, I can see what's going on here. For now I'll tell the shader authors to always add the default case for enum switches since it seems to be handled correctly and there is no reason not to have it.

[damyanp]

@llvm-beanz to see if this is something we need to address in clang. We aren't expecting to fix this in dxc.

[llvm-beanz]

I think we can mark this dormant for DXC. I've filed an issue to remove these instructions during DXIL lowering in Clang.