`process_overrides` may produce invalid `Emit` statements

[andyleiserson]

Found while trying to compact universally in process_overrides for #7638.

This code adjusts emit statements by adjusting the beginning and ending handles for the range. This is correct in cases where expressions have been removed from the module but are otherwise unchanged, i.e. when the range for the emit has shifted backwards. It is not correct if constant evaluation has replaced an expression handle in the range with another handle located elsewhere in the handle space.

A test case for this is the snoise function in the water example:

wgpu/examples/features/src/water/water.wgsl

Line 45 in cf83de3

let C = vec2<f32>(1.0/6.0, 1.0/3.0);

Validation of the module at the conclusion of process_overrides fails with:

Function [4] 'snoise' is invalid:
	Expression [90] can't be introduced - it's already in scope

This occurs because there are multiple emit statements including expression [90].

Note that this shader does not contain any overrides. So there is a question why process_overrides is rewriting expressions. The reason why is that constant evaluation of let statements is inhibited during lowering because (quoting this):

The WGSL spec says that any expression that refers to a let-bound variable is not a const expression. This affects when errors must be reported, so we can't even treat suitable let bindings as constant as an optimization.

However, this same inhibition is not applicable in process_overrides. Perhaps it should be. There are a few possible fixes/changes that could be made here:

1. Inhibit constant evaluation of let statements in process_overrides. Regardless of anything else, this may be necessary for the same reason that it is inhibited during lowering.
2. Skip constant evaluation entirely in process_overrides if the module does not contain overrides, possibly skip everything in process_overrides except the compaction. (This change is included in [naga] Always compact the module in process_overrides #7795.)
3. Fix the logic that adjusts emit statements. I think a problem similar to the above could arise from resolution of override expressions, so while the previous two items might fix this specific case, I don't think they are a complete solution.

[jimblandy]

It is not correct if constant evaluation has replaced an expression handle in the range with another handle located elsewhere in the handle space.

In the front end, at least, constant evaluation just rewrites Expressions before they're added to the arena. Since it happens before appending, it doesn't ever "replace an expression handle" per se. But in evaluating override expressions, we are 'replacing expression handles': we have an existing module and we're changing its code. In that case, naga::back::pipeline_constants::filter_emits_in_block is responsible for updating Emit statements.

If there are multiple Emit statements covering the same expression, that's invalid IR. So isn't the bug in filter_emits_in_block?

[jimblandy]

I believe let statements cannot be override expressions, so we do need to inhibit something, somewhere.

[andyleiserson]

If there are multiple Emit statements covering the same expression, that's invalid IR. So isn't the bug in filter_emits_in_block?

The problem is (just) before filter_emits_in_block, in the call to adjust_block for the function body. The problem is that if you have a fully general mapping of A, B, C, D to X, Y, Z, W, you can't adjust Emit(A..D) by evaluating the mapping on A and D only. You have to evaluate the mapping on all four of A through D, and then ensure the four mapped values are emitted in the output (and not anything else, even if it was inserted somewhere in the middle of X, Y, Z, and W in the output). filter_emits_in_block happens subsequently and may remove the emit for Y if it needs_pre_emit, but at the point the block is given to filter_emits_in_block, it can already be corrupted (irrecoverably, I believe).

There is a question what kinds of mappings ABC->XYZ can be produced in process_overrides. When I first started looking at this problem I was describing it to myself in terms of compaction, but I don't think you can actually get in trouble with compaction alone. Compaction will just remove some expressions, which is fine, since the simple strategy of adjusting Emit(A..D) still works in that case.