Clarify out of bounds behavior by alan-baker · Pull Request #1722 · gpuweb/gpuweb

alan-baker · 2021-05-11T19:11:52Z

Contributes to #1649

Update out of bounds behaviour
- loads can return any in bounds index or 0
- stores can store to any in bounds index or not execute
For pointer and reference, introduces the concept of an invalid memory
reference that covers those behaviours when used
- necessary since the load/store does not occur at the indexing
  expression necessarily

github-actions · 2021-05-11T19:14:18Z

Previews, as seen when this build job started (7410a89):
WebGPU | IDL
WGSL
Explainer

kvark · 2021-05-11T20:31:04Z

       <td>Select the |i|'<sup>th</sup> component of vector<br>
           The first component is at index |i|=0.<br>
-           If |i| is outside the range [0,|N|-1], then an index in the range [0, |N|-1] is used instead.
+           If |i| is outside the range [0,|N|-1], then expression evaluates to either:


same concern about RBA

This is not a memory value so I'm not sure that is appropriate. The vector has been loaded already here and this is then extracting an out of bounds index.

True. So what does Vulkan/SPIR-V say is going to happen in this case?

It produces an undefined value, but I do think we could opt for more restrictive behaviour that is written here (by using a max/clamp operation).

We should discuss this with the group. The way I see it is: RBA's returned value is also pretty much undefined for all the purposes of the user (i.e. the only thing that is defined is that it's not coming from outside of the buffer memory). So if both in-memory and on-stack out-of-bounds accesses are pretty much undefined anyway, why would we prioritize bounds checking (in the spec) one over another?

We talked some more about this internally and it's worth raising here. Most languages distinguish in some way between undefined values and undefined behaviour. This would be an undefined value; however, again generally, undefined values can lead to undefined behaviours. For example, most languages treat branching on an undefined value as producing undefined behaviour. So it might well be worth paying a small cost (the clamp/max operation) to prevent that possibility.

github-actions · 2021-05-12T12:31:36Z

Previews, as seen when this build job started (c193800):
WebGPU | IDL
WGSL
Explainer

dneto0

LGTM

kvark · 2021-05-12T17:46:04Z

       <td>Select the |i|'<sup>th</sup> component of vector<br>
           The first component is at index |i|=0.<br>
-           If |i| is outside the range [0,|N|-1], then an index in the range [0, |N|-1] is used instead.
+           If |i| is outside the range [0,|N|-1], then expression evaluates to either:


True. So what does Vulkan/SPIR-V say is going to happen in this case?

kvark

When this is ready, please make sure to update the PR description (to reflect the actual logic) before merging as it becomes a part of the commit message/history.

kdashg · 2021-05-25T17:44:17Z

WGSL meeting minutes 2021-05-18

(out of bounds for non-memory values)
AB: If you have plain value for a vector, and extract OOB element, what is the result? SPIR-V says “undef value”. But we can do better by throwing in a clamp on the index (for example). E.g. “get one of the in-range values”.
DM: Robust buffer access behaviour is very close to undefined value anyway. If we have this for other pieces for memory access, why introduce a stronger claim here only.
- Can return anything in the bound memory range, or 0-value
JG: Don’t know it’s exactly the bound memory range. Not super confident that RBA will cover case of loading vec3 but reading the 4th element. Get garbage from the register.
AB: To me it’s consistent to say it’s a limited set of values from portability perspective. Saying “undef value” can work.
DM: Saying it’s limited set of result values is misleading. “Anywhere in this bound memory range” is just as bad as “undef value” in practice.
AB: An “undef value” may not be something in memory. It’s worse.
DM: If we do handle that, it’s extra overhead. And in-memory has different results than on-stack (in-register). How do you justify this overhead?
AB: Don’t think you dynamic accesses to values. Think it’s one max.
AB: Not concerned about performance if the shader is doing a bad thing.
DN: Could promise less now, and promise more later.
DM: But you always pay for this.
Consensus seems to be: when getting data out, it’s an “undef value”.
Challenge is to spec that well.
JB: Undef value, undefined behaviour, and unspecified value.
RM: For int and float it’s clear (and include NaN). For bool, result is either true or false.
AB: We don’t specify a bit pattern for bool. Can always
RC: Want to clarify “safe” value. Content from all browser tabs are in the same render process. One tab should not be able to access state outside the security domain of the given.
AB: For memory values, want to bound it to bound memory. Discussion is for non-memory composite values (e.g. let v : vec3<i32> = ..)
RC: WebGL allows you to return 0 or some other value in the buffer. Can’t return value from a different tab’s storage.
AB: Implementation has to avoid abutting storage from different domains too closely (e.g. sharing pages or cache lines). But that’s an implementation detail.
RC: For D3D, arrays on stack, there are no guarantees, and would have to clamp those.
TR: The guarantee is they won’t see memory from another process.That’s not good enough for WebGPU.
AB: So please review the memory part of the PR in question. There’s a bunch of cases you’ve raised that I think are covered in this PR by the pointer and reference parts. Please double check that.
TR: Seems some bounds checking code is needed no matter what. Especially for arrays in local memory.
AB: Yes. And current debate is dynamic index on a let-declared vec3 (for example). For literal index we can do compile error.
TR: Why not define result as 0.
AB: Need to be the union across APIs. “Somewhere in the buffer” is good for Vulkan.
JG: Metal doesn’t give guarantees at all.
TR: Wonder what the difference in performance is between predicating the store vs. allowing the store to go ahead with an out-of-bounds access being wrapped.
JG: Don’t care about perf of a bad program.
TR: I’ve seen code patterns that purposely do OOB to get the 0.
Action on Microsoft to review the memory parts.
Action on AB to update the “undef value” part

kvark · 2021-05-25T17:57:44Z

+If a reference or pointer access is out of bounds, an <dfn noexport>invalid
+memory reference</dfn> is produced.
+[=Load Rule|Loads=] from an invalid reference return one of:
+    * a value from any [=memory locations|memory location=] of the [=originating variable=]


Is the use of "originating variable" appropriate here? I.e. if I have a struct Foo {...} used as a uniform buffer, and we are talking about access into this buffer, would the originating variable be that global var<uniform> foo: Foo?

If so, this is still incorrect. Vulkan's RBA allows any value from the buffer (bound from the host side) to show up here, not just Foo contents.

I made a change to refer to the bound buffer instead of just the originating variable.

kvark · 2021-05-25T18:01:04Z

+[=Load Rule|Loads=] from an invalid reference return one of:
+    * a value from any [=memory locations|memory location=] of the [=originating variable=]
+    * the zero value for store type of the reference
+    * if the loaded value is a vector, the value (0, 0, 0, x), where x is:


may need a bit of clarify (possibly in follow-up) on, like, what if it's vec2. Maybe we should just say here "if the loaded value is vec4?

kdashg · 2021-06-01T07:54:30Z

WGSL meeting minutes 2021-05-25

DM: Uses semantics of “orginating variable”. Which is WSGL-centric. Have to consider the host-side. Consider the resource which is the source for the binding.
MM: But the point is you won’t see data from another resource. Use case is you might multiplex multiple user contexts into a single graphics context? Is that the point.
DM: Think they want to limit it as much as possible.
RC: For each access you want to know which buffer is being accessed.
MM: What happens on Vulkan with bindless.
Metal guarantees no access reaches outside the process.
Same for Windows
Discussion about separating WebGPU context per process
- Has high cost. E.g. sharing with image sources, compositing.
- Firefox shared compositing resources across processes
RC: Reaching within process but outside resources can land on a page you own (safe), or not own (which leads to crash). That’s “safe” but non-portable and painful to deal with.

kdashg · 2021-06-01T20:10:20Z

WGSL meeting minutes 2021-06-01

DM: Problem was with concept of “originating variable”, but robustness only guarantees within-buffer, not within-binding.
AB: Can change that.
AB: Also need a review from Microsoft.
RC: I skimmed over it. Thought it was ok, but agreed with DM’s point about “originating variable”.
AB: Some of the answers will change, depending on where we end up with array and matrix accessing.
JG: Myles mentioned that Metal requires within-binding, but MSL doesn’t have robust-access behavior.
RC: Is there provision to allow returning zero if it’s OOB?
AB: Yes. Within buffer or return zero.
RC: With that, seems ok with me. Greg?
GR: Need time to review.
TR: Need time to review.

Contributes to gpuweb#1649 * Update out of bounds behaviour to allow D3D and Vulkan RBA behaviour * For references, introduce an invalid memory reference * propagates through indirection and address of operators * loads can return any value in the buffer, 0, or (0,0,0,x) * stores can store to any location in the buffer or not be executed * for values, the accessed value is undefined

* The load/store may access any location(s) of the bound buffer, not just the originating variable

@kvark

SHA: ce3eb2c Reason: push, by @kvark Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@kvark

SHA: ce3eb2c Reason: push, by @kvark Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

pow2clk · 2021-06-11T19:10:22Z

Sorry I didn't directly respond to this, but I did look it over. The provisions for out of bounds access seem broad enough that they aren't of concern.

I share the concern @kvark expressed here https://github.com/gpuweb/gpuweb/pull/1722/files#r639050496, but I suppose that will be address later

alan-baker added the wgsl WebGPU Shading Language Issues label May 11, 2021

alan-baker requested review from dneto0, kvark and litherum May 11, 2021 19:11

dneto0 reviewed May 11, 2021

View reviewed changes

Comment thread wgsl/index.bs

kvark requested changes May 11, 2021

View reviewed changes

dneto0 approved these changes May 12, 2021

View reviewed changes

kvark reviewed May 12, 2021

View reviewed changes

Kangz mentioned this pull request May 19, 2021

Out-of-bounds clamping brings big perf regression on TFJS ML demos. #1202

Open

alan-baker requested a review from pow2clk May 19, 2021 13:44

kvark requested changes May 19, 2021

View reviewed changes

Comment thread wgsl/index.bs

Comment thread wgsl/index.bs Outdated

alan-baker force-pushed the out-of-bounds branch from cc99e89 to cb74a6f Compare May 20, 2021 13:46

kvark reviewed May 25, 2021

View reviewed changes

alan-baker added 2 commits June 9, 2021 15:13

Update invalid memory reference behaviour

753a184

* The load/store may access any location(s) of the bound buffer, not just the originating variable

alan-baker force-pushed the out-of-bounds branch from 1d94897 to 753a184 Compare June 9, 2021 19:14

kvark approved these changes Jun 10, 2021

View reviewed changes

kvark merged commit ce3eb2c into gpuweb:main Jun 10, 2021

github-actions Bot added a commit that referenced this pull request Jun 10, 2021

Clarify out of bounds behavior (#1722)

f16f78f

SHA: ce3eb2c Reason: push, by @kvark Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions Bot added a commit that referenced this pull request Jun 10, 2021

Clarify out of bounds behavior (#1722)

12824a8

SHA: ce3eb2c Reason: push, by @kvark Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

kainino0x mentioned this pull request Jun 11, 2021

Define index/vertex buffer OOB behavior #1835

Merged

This was referenced Jun 15, 2021

Buffer indices should be unsigned #1135

Closed

wgsl: limiting undefined behaviour; subdividing "dynamic error" #1844

Closed

ben-clayton mentioned this pull request Jun 30, 2021

OOB behaviour for storage textures #1895

Closed

alan-baker deleted the out-of-bounds branch August 23, 2021 19:31

Conversation

alan-baker commented May 11, 2021

Uh oh!

github-actions Bot commented May 11, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 12, 2021

Uh oh!

dneto0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kdashg commented May 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdashg commented Jun 1, 2021

Uh oh!

kdashg commented Jun 1, 2021

Uh oh!

pow2clk commented Jun 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants