SPIRV: Remapper: allow 64 bit literals in OperandVariableLiteralId iteration

[ghost]

This PR addresses issue #435, where iteration over the OperandVariableLiteralId argument list class would not cope with multiple word literals.

It also includes a small improvement to dead type elimination which picks up some cases previously dropped. The algorithm used for this is suboptimal, but it's an offline tool, it was easy to write that way, and it can be further improved later if there is ever a performance concern.

[dneto0]

The "word-2" expression is strongly tied to the fact that this must be an OpSwitch instruction. It just so happens that OpSwitch is the only case so far where we have an OperandVariableLiteralId.
I'd probably assert that opcode == OpSwitch.

[ghost]

Good point. Will change.

[dneto0]

Works for me.

The only other place in the grammar where Ids and literals are mixed is in OpGroupMemberDecorate, but there the literals are the 32-bit index of the structure member. So this problem won't occur in that context.

This patch works on the submitted case, and also in the following, were I made OpConstant for 64-bit integer and 64-bit float:

               OpCapability Shader
               OpMemoryModel Logical GLSL450
       %void = OpTypeVoid
       %long = OpTypeInt 64 1
     %double = OpTypeFloat 64
          %4 = OpConstant %double 3.14
          %5 = OpConstant %long 5000000000
          %6 = OpTypeFunction %void
          %7 = OpConstant %long 0
          %8 = OpFunction %void None %6
          %9 = OpLabel
               OpSelectionMerge %10 None
               OpSwitch %7 %10 0 %11 1 %12
         %11 = OpLabel
               OpBranch %10
         %12 = OpLabel
               OpBranch %10
         %10 = OpLabel
               OpReturn
               OpFunctionEnd

[ghost]

Cool; thanks for the review. I've pushed a new version with your review changes.

[dneto0]

Ooops. This is causing a failure in the Vulkan CTS.
I get an abnormal exit in vk-build-programs --deqp-case=dEQP-VK.glsl.constant_expressions.other.switch_case_fragment

[dneto0]

This is the failing test case, in GLSL form.
When I compile this with -V and then run it through the remapper, I get an internal error message:
"ID not found".

#version 310 es
//;dEQP-VK.glsl.constant_expressions.other.switch_case_fragment
precision highp float;
bool isOk (int a, int b)     { return (a == b); }
layout(location = 0) out mediump vec4 dEQP_FragColor;
layout(location = 0) flat in float in0;
layout(binding = 0, std140) uniform Reference
{
        int out0;
} ref;
int out0;


void main()
{
        const int _0 = 0;
        const int _1 = 1;
        const int _2 = 2;
        const int _3 = 3;
        const int _4 = 4;

        switch(int(in0))
        {
                case _0:
                        out0 = 0;
                        break;
                case _1:
                        out0 = 1;
                        break;
                case _2:
                        out0 = 2;
                        break;
                case _3:
                        out0 = 3;
                        break;
                case _4:
                        out0 = 4;
                        break;
                case 5:
                        out0 = 10;
                        break;
                default:
                        out0 = 100;

        }
        bool RES = isOk(out0, ref.out0);
dEQP_FragColor = vec4(RES, RES, RES, 1.0);

}

[dneto0]

I've narrowed it down to the --map types option

[dneto0]

If I use --map names then I end up with "unimplemented type size request". So there's something deeply bad going on.

[dneto0]

With --dce-types I get "ID not found" as well.
I've requested that this PR be backed out.

[johnkslang]

It's out.

Definitely need a better testing strategy.

[johnkslang]

Or, something like this could be experimented with in a branch first, like the Vulkan precision qualifier change.

[ghost]

Thanks for the reproduction case; I'll check it out.

[ghost]

I see the problem; it relates to how I was obtaining the type size from the instruction stream during the remapping pass, which is actively changing all the IDs.

I'll fix. Unless you have an urgent need, I'd like to do it next week rather than when tired at the end of this one.

[ghost]

Also, I'll focus soon on creating a better testing infrastructure, since my test dataset didn't expose the problem. That might take a bit longer since it seems best coupled with other infrastructure changes if it's to use SPIRV-Tools for testing.