o3de / o3de-azslc

Amazon Shader Language (AZSL) Compiler
Other
23 stars 14 forks source link

option range not reconstructed #75

Closed siliconvoodoo closed 1 year ago

siliconvoodoo commented 1 year ago

While investigating https://github.com/o3de/o3de/issues/13625 I stumbled into the possibility that option with min != 0 never really having worked ever.

This is my basis for the claim: Take this input program:

ShaderResourceGroupSemantic slot1
{
    FrequencyId = 1;
    ShaderVariantFallback = 128;
};

ShaderResourceGroup SRG : slot1
{
};

[[range(10,11)]]
option int o_stuff = 10;

option enum E { v = 10, v2 } o_s2;

float4 MainPS(float2 uv : TEXCOORD0) : SV_Target0
{
    return o_stuff;
}

The getter functions look like this:

#if defined(o_stuff_OPTION_DEF)
    static const int o_stuff = o_stuff_OPTION_DEF ;
#else
    static const int o_stuff = GetShaderVariantKey_o_stuff();
#endif

#if defined(o_s2_OPTION_DEF)
    static const ::E o_s2 = o_s2_OPTION_DEF ;
#else
    static const ::E o_s2 = GetShaderVariantKey_o_s2();
#endif

// ... code

int GetShaderVariantKey_o_stuff()
{
    uint shaderKey = (::SRG_SRGConstantBuffer.SRG_m_SHADER_VARIANT_KEY_NAME_[0].x >> 0) & 1;
    return (int) shaderKey;
}

::E GetShaderVariantKey_o_s2()
{
    uint shaderKey = (::SRG_SRGConstantBuffer.SRG_m_SHADER_VARIANT_KEY_NAME_[0].x >> 1) & 1;
    return (::E) shaderKey;
}

I think we can mathematically prove that the values coming out of GetShaderVariantKey_o_stuff are 0 or 1. Because it's and'd with 1. On the C++ counterpart of the option management system, we see a lot of this: EncodeBits(group.GetShaderVariantKey(), valueIndex.GetIndex() - m_minValue.GetIndex());

Which hints that the min is NOT encoded in the key bits. Which is logical since we are doing compression. But, if we do so, we need to add the min at reconstruction, otherwise the contract about the range is not respected, we destroy the values by flooring them back to a 0-based range. I believe this is a bug, even though I have not attempted an empirical (observed) reproduction yet.