SPIR-V back end skips bounds checks for `AccessIndex` when applied to runtime-sized arrays

[jimblandy]

Naga's SPIR-V back end fails to generate a bounds check for an AccessIndex expression that is indexing into a runtime-sized array.

For example:

[[block]]
struct Unsized {
    arr: array<f32>;
};

[[group(0), binding(0)]] var<storage> unsized: Unsized;

fn fetch(i: i32) -> f32 {
   return unsized.arr[3] + unsized.arr[i];
}

When the ReadZeroSkipWrite policy is applied to buffers, Naga generates the following SPIR-V for the body:

      %fetch = OpFunction %float None %13
          %i = OpFunctionParameter %int
         %10 = OpLabel
               OpBranch %14
         %14 = OpLabel
         %20 = OpAccessChain %_ptr_StorageBuffer_float %unsized %uint_0 %uint_3
         %21 = OpLoad %float %20
         %22 = OpArrayLength %uint %unsized 0
         %23 = OpULessThan %bool %i %22
               OpSelectionMerge %27 None
               OpBranchConditional %23 %28 %27
         %28 = OpLabel
         %25 = OpAccessChain %_ptr_StorageBuffer_float %unsized %uint_0 %i
         %29 = OpLoad %float %25
               OpBranch %27
         %27 = OpLabel
         %30 = OpPhi %float %26 %14 %29 %28
         %31 = OpFAdd %float %21 %30
               OpReturnValue %31
               OpFunctionEnd

Note that the the unsized.arr[3] access has no bounds check, while the unsized.arr[i] access does. Since it is not known until run time whether 3 is a valid index, both accesses should be checked.

Two factors limit the impact of this bug:

• The problem is restricted to runtime-sized arrays: the validator performs bounds checks for AccessIndex expressions applied to any type whose length is statically known.
• We expect that many targets will support a form of robust buffer access that would make bounds checks unnecessary on arrays in the storage storage class, which is the only storage class that can hold dynamically sized arrays.

However, Naga is expected to be used in situations where that kind of robust buffer access is not supported, so this is still important to fix.

[kvark]

This is basically #1803. We'd want to make it AccessIndex and do the bounds check right away, but Alan from Google feels that the out-of-bounds for this should be handled without any errors, like for dynamic access. That is what I disagree with.

[jimblandy]

Oh, gpuweb/gpuweb#1803?

That's related, but I think this is just a flat-out bug. If a front end uses AccessIndex on a dynamically-sized array, we're not doing the bounds checking anywhere - not at shader creation time, pipeline creation time, nor dynamically.