Fix Intel iGPU Rendering

[tychedelia]

Fixes #18904.

See also:

• Naga front ends do not properly short-circuit && and || gfx-rs/wgpu#4394
• [naga wgsl-in] Short-circuiting of && and || operators gfx-rs/wgpu#7339

A Debugging Story

In the above issue, the defect can be reproduced by running the 3d_scene example with an Intel adapter on Vulkan and observing the following output:

1. Indirect Draw Args

The first thing to check is what's being passed to vkCmdDrawIndexedIndirectCount in main_opaque_pass_3d. What we see is a little bizarre:

We should see two separate meshes here for the circular base and the cube, but instead we see 3 items, two of which are identical with exception of their first_instance id.

2. Reversed work item ordering

Trying to debug what could possibly be wrong with the shader input lead to observing the PreprocessWorkItem buffer that gets passed to mesh preprocessing. Here, there was a very conspicuous difference between when things would work and when they would break:

// Working
work_items[0] = {input_index: 0, output_index: 0}
work_items[1] = {input_index: 1, output_index: 1}

// Broken
work_items[0] = {input_index: 1, output_index: 0}
work_items[1] = {input_index: 0, output_index: 1}

This was very conspicuous and likely due to ECS query instability. However, the code looked like it should be robust enough to handle work items in any order. Further, this works just fine on Nvidia, so the ordering itself is clearly not the issue.

3. Memory ordering?

My first assumption was that this must be some kind of weirdness with memory ordering or some other race only observable on Intel. This led me to the following write:

    // If we're the first mesh instance in this batch, write the index of our
    // `MeshInput` into the appropriate slot so that the indirect parameters
    // building shader can access it.
    if (instance_index == 0u || work_items[instance_index - 1].output_or_indirect_parameters_index != indirect_parameters_index) {
        indirect_parameters_gpu_metadata[indirect_parameters_index].mesh_index = input_index;
    }

Even though the logic looks totally fine and shouldn't require any synchronization, I tried making the write atomic, which didn't seem to help at all. My next step was to try to remove the condition and just unconditionally write. This could lead to some weirdness in the event of batch size N > 1, but I just wanted to see what happened. And,... it solved the bug?

4. SPIR-V de-compilation

This made no sense to me why this would fix things, so I decided to decompile the shader and see what was going on, and found the following:

  bool _247 = _236 == 0;
  uint _248 = _236 - 1;
  uint* _249 = &_221[_248].output_or_indirect_parameters_index;
  uint _250 = *_249;
  bool _251 = _250 != _246;
  bool _252 = _247 || _251;
  
  if(_252) {
    uint* _256 = &_227[_246].mesh_index;
    *_256 = _244;
  }

This looks wrong. The final condition _247 || _251 doesn't seem right. Checking and confirming, || in WGSL is supposed to short circuit. Instead, here, we unconditionally read from &_221[_248].output_or_indirect_parameters_index; AKA work_items[instance_index - 1]. In the event instance_index is 0 that means we are OOB. Uh oh!!

5. Vendor differences

I'm not sure why this UB have any effect on Nvidia. But we can walk through the entire bug:

On the first thread in the workgroup where instance_index is 0, we will always OOB read, which on Intel seems to cause the thread to terminate or otherwise return garbage that makes the condition fail or something else weird. However, in the event that the first work item's input/output index is supposed to be 0, everything happens to just work, since the zero-initialized memory of the GPU metadata is by chance correct. Thus, the bug only appears when things are sorted funny and a batch set with a non-zero input index appears at the front of the work items. Yikes!

The fix is to make sure that we only read from the prior input when we are not the first item.

[jf908]

Tested it on 2 machines (Intel i5-6200U and Intel i7-12700H) and can confirm that this PR fixes the issue!

Excellent debugging, explanation makes sense, the shader logic is indeed the same which confirms this isn't a bug with the logic but something further down the line.

[DGriffin91]

Tested on i7-9700 CPU with UHD Graphics 630.

[andyleiserson]

Welp, I am really sorry that you had to debug this 😞.