Skip to content

nir_loop_analyze: Fix iteration count guessing for loops with i <= limit

Jesse Natalie requested to merge jenatali/mesa:loop-analysis into main

Given a shader of the form:

shared uint data[4];
void foo(int limit) {
    for (int i = 1; i <= limit; ++i) {
        data[i] = /* some expression */;
    }
}

Loop analysis was incorrectly guessing that this loop could iterate 4 times. In essence, it used the fact that data was accessed using the induction variable, indicating that the highest value for the induction variable would be 3. It correctly realized that taking i from 1 to 3 would result in 3 loop iterations. But, then ran a final step where it attempted to substitute values and re-run the comparison - which I don't entirely understand, to be honest. In that final step, it essentially was replacing limit with a value of 4, since 4 was the array size... but since the loop used a <= check, it incorrectly determined that i = 4 was a valid iteration.

This is all problematic for us because the DXIL validator considers that resulting shared array access with an out-of-bounds literal index to be invalid, and refuses to sign the shader. Obviously in practice, limit would never be >= 4 so that code couldn't execute. But still, we shouldn't generate the code if it's invalid.

Note that with this, after "partial" unrolling, we'll still generate a final loop. This is because the loop unrolling pass doesn't take the initial value of the induction variable into account, nor the direction/step of the induction variable. It just checks "the array is accessed with the induction variable as an index" and "N loop iterations were unrolled" and assumes that future accesses must start from index N, so only does a bounds check on N. In this case, we've unrolled 3 iterations, but future accesses must start from 4, not 3, and so the array accesses in the loop remainder can be removed. Not a problem in practice because this code won't execute, but it should be deletable if the unrolling pass was smarter.

Edited by Jesse Natalie

Merge request reports