Error with literal indexing in a shader

This is a follow-up to #1241, which got closed because @dneto0 landed a generic infrastructure to handling errors in 3 different places. But one particular case, which was discussed there (see https://github.com/gpuweb/gpuweb/issues/1241#issuecomment-772718481) is not covered.

What if I do vector[5] in the shader. Should the implementation error on shader module creation, pipeline creation, or either of them?

I'm not 100% clear if this is an array named "vector" or an access on a vector value.

Let's make this concrete:

fn foo() {
   var A: array<i32,4>;
   var V: vec<i32,4>;
   let a5 = A[5];    // 
   let v5 = V[5];
}

Both these fall into the (2) case from your comment: https://github.com/gpuweb/gpuweb/issues/1241#issuecomment-772718481

That is, use runtime boundscheck on this, don't reject this shader.

In the spec, all indexed access cases {vector, matrix, array}x{ref,value} have the "If i is outside the range..." language." So I think the spec already treats it as your (2) case.

Do you want to change that?

Both these fall into the (2) Do you want to change that?

I still stand by my past comment in principle:

separate two types of accesses statically:

• when the index is known at compile time
• when index is not known

But the details (the second part) become outdated since then. First, letter access to matrices never became a thing. Second, the spec got the notion of "const_expr", which is now affecting the semantics of accesses with #1801.

So the details need to be refreshed with this in mind. I think indexing with const expr outside of bounds should be allowed to be erroring at compile time.

So the details need to be refreshed with this in mind. I think indexing with const expr outside of bounds should be allowed to be erroring at compile time.

Fair. In any case, these out-of-bounds cases should be explicitly spelled out as one of the 3 classes of error. So there's something to update in the spec anyway.

One thing to consider: with #1792, array sizes may not settle until pipeline creation time. So that makes trying for 'index known at compile time' only rejectable at pipeline creation time.

A viable and simple option is: vector indexed via const_expr out of bounds --> shader creation error array indexed via const_expr out out of bounds -> pipeline creation error (but needs potentially many checks at this time)

I suppose you could split the array cases where the element count is shader-creation-time constant vs. pipeline-creation-time-constant. I'm not sure it's worth the complexity

Sorry, I should have expressed my preference:

vector indexed via const_expr out of bounds --> shader creation error array indexed via const_expr out out of bounds -> dynamic error (keeps it simple)

Consensus from 2021-06-08 is:

• treat it as a dynamic issue rather than static.
• still can use a clarification in the spec that even though all the index and sizing information may be available at shader creation time, it is handled as a dynamic issue

WGSL meeting minutes 2021-06-08

• MM: Motivation: It would be bad if one browser had different choices of what to reject.
• DN: Yes, we just need to pick one and standardize. My pref is to tread array case as dynamic, even for literals. Would be fine having static error for vectors.
• MM: Aren’t arrays not harder than vectors, expect dynamic arrays.
• DM: Maybe we should punt on that?
• MM: Also, from a compiler laziness standpoint, nice to treat everything as dynamic.
• DN: Nice to have single universal rules.
• JG: Isn’t vector subscripting have the same limits as matrix subscripting?
• JB: There are more useful operators for vectors here than for matrices.
• (consensus: Treat it as dynamic)

@dneto0 we talked about this internally, and I think there is a problem with the consensus. If [] syntax is only for dynamic indexing, then by-value arrays become non-indexable. Previously, #1801 changed the semantics of indexing in a way that only allows matrix and array dynamic indexing behind a reference. For matrices, we still have an option to add letter access. But of arrays, it's only the [] brackets. This means, any access to an array is a dynamic access, if we go this route. In turn, that means by-value arrays can't be indexed in WGSL, even though static indexing could be safely translated into OpCompositeExtract.

This means, any access to an array is a dynamic access, if we go this route. In turn, that means by-value arrays can't be indexed in WGSL

Not quite. It's the error case that is classified as a dynamic error. The access itself still is done with a literal, but one that is in bounds. In the meeting @litherum checked the understanding by referencing the example given above:

If the program is written like this:

fn foo() {
   var A: array<i32,4>;
   var V: vec<i32,4>;
   let a5 = A[5];    
   let v5 = V[5];
}

Then the shader is allowed, and the implementation may translate it into something like this:

fn foo() {
   var A: array<i32,4>;
   var V: vec<i32,4>;
   let a5 = A[0];    //  Use an in-bound index. This is one choice.
   let v5 = V[3];   // This is another choice.  Or whatever else is allowed by #1722
}

Now, I feel a little silly writing the above. It sure reads like a mis-compile, even though it's (what I understand) is the consensus. This is one case where I sure would like to have a warning from tooling to say this has occurred (and a warnings-as-errors mode).

@dneto0 sorry, I think you misunderstood the concern, which I explained poorly. Let me try again.

So we appear to talk about 2 different notions of "dynamic access", that is - an access into a composite type where the index is not statically known at compilation time:

1. used for the subject - checking out-of-bound accesses and triggering errors at compile time (for static access) versus ... no time (just pretending everything is OK and patching the shader code)
2. used to determine if it's valid to use the [] operator on a matrix or an array, according to #1801

I think splitting the 2 notions would be harmful to the spec and would cause a lot of confusion. Let's try to find a solution that leaves only a single definition of what is "dynamic access" in the spec, and applies it to both (1) and (2) usages.

Concrete example that tripped one of the workflows for us:

let _e21: mat4x4<f32> = global1.Model;
mat3x3<f32>(_e21[0].xyz, _e21[1].xyz, _e21[2].xyz)

If [] is considered "dynamic access", then this code is invalid WGSL, since by-value matrices can't be dynamically indexed.

At some point, didn't we have a proposal for mat.x? E.g. mat3x3<f32>(mat4x4.x.xyz, mat4x4.y.xyz, mat4x4.z.xyz)

At some point, didn't we have a proposal for mat.x?

I thought @kvark had an open issue for it, but I can't find it.

I don't think the matrix access by letter is a solution here. It allows "static" access into matrices, but there are still fixed-size arrays left on the table. If I understand correctly the discussion so far, we've gone through the following steps:

1. the problem is identified. Users want OpCompositeExtract, but instead they get a validation error.
2. I request the group to unify the policy of "static" access between the validator and out of bounds handler.
3. The opposing point of view is that for the bounds checking it would be good to leave the "static" case detection to be an implementation detail, and thus it's up to the implementation to use OpCompositeExtract as a possible optimization. (did I get this point right?)
4. To which my response was that we still have to detect the static access for the matter of validation. In other words, the validation rules make it 100% predetermined when an implementation can do OpCompositeExtract or not. Therefore, it doesn't have to be an implementation detail, and we can have those "static" definitions unified.

Removing from the API project tracker as I think this can only be figured out in the WGSL group. The change in where the error surfaces is incidental.

We discussed this a bit in the office hours. It's possible that some context is lost, given that this was previously discussed 5+ weeks ago, but the general consensus is:

For const_expression indexing of a value array (and that's the only allowed indexing), there is no point in having the out-of-bounds behavior loosely defined. SPIR-V backend has to generate OpCompositeExtract, which uses a statically known index, so the implementation can figure out what to do, and we can agree on the behavior. For example:

• we can agree on DX12-like behavior there, where reads produce zeroes and writes are ignored
• we can make this an error at createShaderModule time, since it's not useful for either users or implementors

we can make this an error at createShaderModule time, since it's not useful for either users or implementors

I prefer this option.

I'd also prefer erroring at shader module creation time.

For const_expression indexes, I don't see a reason why we shouldn't do this for all arrays/matrices/vectors, not just just those using let.

IIRC, @dneto0 also preferred this to be an error. So it's pretty much "needs specification" now.

After further discussion, we don't have consensus inside Google. (Sorry)

• @alan-baker still thinks its better for the spec to have a consistent rule for invalid array accesses.
  • Will this be extended to constexpr, and potentially overridable (if/when).
  • Not necessary for MVP

WGSL meeting minutes 2021-09-07

• DN: AB strongly wants us to have consistent behavior for OOB access here, so more discussion is needed here.
• DM: The method for OOB protection is in fact different for different types here, so I’m not sure we need them to all be the same.
• RM: While I think consistency is important for portability, I think it’s fine to have this be compile-time.
• MM: Whenever we expand constexpr, if we do this analysis, this should apply to all indexing, not just literals.
• DM: Right, it’s not that it’s an option not to for e.g. let-arrays and OpCompositeExtract
• MM: I think the job for the compiler is the same.
• MM: DM, can you restate the OOB behavior point?
• DM: On uniform and storage memory backed arrays are relying on robust-buffer-access, but everything else uses manual code injections for guarding against OOB access. Point AB was making was that we should have consistent behavior between these different types. I’m saying that we’re already doing something different, so why pretend we’re not.
• MM: So the consistency argument isn’t particularly strong for you here.
• DM: Two types of consistency here, so it’s not clear-cut.
• DN: Today, spec gives you an Invalid Reference if you reach outside your array. If this is still inside the robust-buffer-access, the hardware feature will not necessarily kick in to save you. I haven’t thought it all the way through, but I worry about us being incomplete here.
• MM: Ok, so you have a buffer of ten-element arrays. I think it’d be bad if it had to do access checks against both the ten-element array, and also against the full buffer/outside array. If we only have one for performance reasons, we only want the latter.
• DM: I think that’s actually what we have now.
• DM: I think we might have spec’d something even less restrictive, maybe for local variables? We should double-check that we haven’t written divergent things.
• MM: Can we tentatively resolve and ask for reversion if need be?
• DN: Would prefer to get input first, preferably in written async form.

As I understand this issue there is a tension between:

• being consistent between different types of accesses (e.g. a dynamic indexing into a dynamically typed array vs a static indexing into a vector/constant-sized array)
• providing good error messages where possible, and not emitting silly code (e.g. emitting code that we know will always fail at runtime).

I do not see the first item as being more important than the second, so I am weakly in favor of having compile-time errors for the case where the array is fixed-size or a vector, and the index is a constexpr.

The main concerns to me regarding this were portability and implementation difficulty. I believe that portability is unlikely to be a problem as long as we clearly specify which cases fall into the compile-time error case (with a clear definition of constexpr) and which cases fall into the runtime behavior case. I have also been convinced by the talk during today's meeting that the implementation difficulty is likely to be minimal as most implementations already have to know which kind of indexing is going on for compiling to targets like SPIRV properly.

WGSL meeting minutes 2021-09-14

• AB: The rules are consistent now. There is tension if you want to change them: if you want constexpr to come in now, and if you want to implement using SPIR-V spec constants, then you can’t evaluate them in the browser's code. It’s unclear what the compilation path is on the different platforms. You get inconsistent constraints on when things can be evaluated. There are consistency issues when you get beyond literals.
• RM: We already have different rules about where constexpr/literals can be used (e.g. offsets).
• AB: It’s been raised that some people would want to use SPIR-V spec constants. Why double-up to implement the expressions in two places. If you want to allow spec constants as the implementation then you don’t want this as a programming error. Let the check be deferred.
• DM: SPIR-V spec constants can’t implement all of desirable constexpr, e.g. cosine. Then why have two paths. Are you sure you want to use spec constants in SPIR-V for anything?
• AB: Depends on how far you want to go with constexpr. Not clear that I want to evaluate floating point math in the browser. That’s not the real value. Far more value in just integer expressions, which maps nicely to SPIR-V spec constants.
• DM: So you say it depends on scope.
• AB: And also requirements on the implementation about when they are evaluated.
• DM: Can you use OpConstantExtract with a spec id as ? Thought you can’t.
• AB: It’s one of the opcodes allowed for OpSpecConstantOp.
• DM: I’m talking about the indices into a value-array. You can’t use access chain. Have to use OpCompositeExtract. You have to evaluate that index on the spot as a literal.
• AB: That may be desirable, but is not the predefined only way. Gets tied up with the rules for indexing into value-array
• AB: If we get more clear about where the evaluation must occur, then it would be more palatable. (.... missed stuff….)
• JG: Would be happy to make it a warning; it’s a pretty good signal it’s a bug.
• DM: We decide based on how things are implemented. But we don’t have the whole picture there.
• JG: Difference here is constexpr is not mandated that it must be implemented a certain way. Usually you want it all done at compile time. Even in the C++ standard it’s as-if behaviour. The actual thing the machine does isn’t what you expect it to. But if we go too far here, we might back ourselves into a corner into how it’s implemented.
• JG: Premise: because we’re not 100% sure we want to constrain ourselves to always evaluate these early, then we should leave the door open to evaluating them later. Onus is to provide argument/evidence that it’s ok to force the earlier.
• AB: We have difference of opinion on what’s efficient in SPIR-V w.r.t. CompositeExtract vs. AccessChain on a local variable. That’s a different issue. Let’s not rat-hole on that here.
• DM: One issue depends on the other. The let-arrays, constexpr, and this. One of them has to be a leaf to help resolve the other ones.
• AB: This is the smallest of them. The other issue should be decided first. For Const-expr we should decide on scope. This decision falls out of those.
• DM: constexpr scope is slippery. How big do you go? Goes circular.
• JG: Guess and iterate.

I think there might be consensus here that making this a hard error might not be required for mvp/v1, but I want to check with the group during the meeting.

WGSL meeting minutes 2021-09-21

• JG: Suggest not making this a hard error, but can make it hard error later
• MM: Not my recollection. My recollection is what Robin said. https://github.com/gpuweb/gpuweb/issues/1803#issuecomment-914826793 (consistent/what-makes-sense)
• JG: think we think differently about what makes sense/consistent.
• DM: Alan had counterargument. Want them written.
• AB: Think the notes from last week captured it. https://github.com/gpuweb/gpuweb/issues/1803#issuecomment-920211977
  • Would want to first converge on the breadth/depth of the feature of constexpr. When must constants be folded. This issue is the tail of the dog, but the “scope” of constexpr feature is the dog, i.e. more important to consciously decide on that first.

Added to the pile of things "depends on constexpr semantics"

WGSL meeting minutes 2022-05-17

• AB: As stands, it’s an undefined value out of bounds. Any value is possible for that type. Folks have expressed static values should be shader compiler error. Deferred to creation time expressions which we have. Could categorize as if creation time expression it’s shader creation error otherwise standard out of bounds behaviour defined elsewhere. Personally not hugely satisfying but if folks like can write it up.
• JB: What do you find more satisfying?
• AB: Leaving as is. Not a big deal as we’ve defined the behaviour. Tooling could add warnings but making a hard error is making a decision for someone
• KG: Included towards that. Symmetric and consistent across constexpr and dynamic access. Easy to implement compile wise. If have and confused emit 0 and a warning. If that makes you more satisfied makes me too
• JB: Not going to jump up and down to flag as an error at compile time.
• KG: Sounds like consensus on that and we can revisit later if needed
• JB: Getting errors from FXC where it unrolls loops and detects dynamic out of bounds which we’ll never detect. It’s annoying so we don’t want to throw that at other folks.
• TR: FXC is weird here. Like FXC unrolling and going out of bounds on an infinite loop. Not sure how to deal with that generating HLSL and going to FXC
• GR: DXC doesn't’ do that
• JP: How undefined is hte undefined result? Is a branch non-uniform?
• KG: We’ll commit to saying it’s uniform. Should give a uniform value.
• JB: What if index is not uniform
• KG: Then non-uniform. Feel like accidentally hitting trip wire of this being out of bounds should not change uniformity.
• AB: That’s fair
• JP: Then do we need different language or is dynamic out of bounds always uniform?
• AB: If same expression we shouldn’t analyze it differently.
• JB: The FXC case is coming from community case of WGPU.
• AB: Assign to me and I’ll cleanup.

See also #3253

#3400 landed the change to make this case an error. Closing.