Apply extended robust access on global buffer access #2874
Conversation
Test summary for commit d339cafCTS tests (Failed: 0/138443)
Ubuntu navi3x, SrdcvkUbuntu navi2x, Srdcvk |
There was a problem hiding this comment.
Thanks! The general flow of this makes sense to me, though I do have a couple of comments.
Typo in the title: *descriptor
We have some freedom in how we choose to lower the various access and bounds checks. The pre-existing code avoids the branch by overriding the offset check, which makes a lot of sense in general, but perhaps not so much when we have a branch anyway: the bounds check could become part of the branch check. Have you thought through the implications?
It's not actually clear that this is always an improvement. In particular, what happens if we have a sequence of related loads and stores to/from the same buffer? Can we optimize that somehow?
These two points are something for you to think about if you haven't yet, though they don't necessarily have to affect this PR.
The pre-existing code by overriding the offset as 0 cannot work well for both null descriptor and out-of-bound. I will merge the oob and null desc as the branch check as you said. I have two concerns:
Yes, the branch will impact the optimization on merging a sequence of loads/stores. Unfortunately, We have to add it to ensure correct behavior in the case of null descriptor allowed. This pr is to resolve a TDR issue in DX app due to accessing null descriptor. |
|
V1: renaming |
Test summary for commit 586f50bCTS tests (Failed: 134/138378)
Ubuntu navi3x, SrdcvkUbuntu navi2x, Srdcvk |
|
V2: fix tests failing. |
xuechen417
force-pushed
the
nullDesc
branch
2 times, most recently
from
7d87f10
to
c2ed376
Compare
Running LoadStoreVectorizer before PatchBufferOp may help on part of this. To further improve that, we may need rework of PatchBufferOp to allow lowering a group of instructions, but it seems nontrivial effort. |
Test summary for commit 7d87f10CTS tests (Failed: 0/138378)
Ubuntu navi3x, SrdcvkUbuntu navi2x, Srdcvk |
Test summary for commit c2ed376CTS tests (Failed: 0/138378)
Ubuntu navi3x, SrdcvkUbuntu navi2x, Srdcvk |
Applications which don't need the extended |
Good point. Checking dword2 (num_records) is probably enough. At least, I can't think of a reason why it wouldn't be right now. |
|
V2: Use dword2 for null descriptor and add out-of-bound as the branch check if it is needed by the app. |
xuechen417
changed the title
This change will perform defined behavior for global buffer access in the case of null descriptor or out-of-bound if the application need the extended `robustnessAccess`. NOTE: we use dword2 to check null descriptor.
|
Did you push anything? :) |
Oops! Thanks for reminding. Finish pushing. Please help review. |
Test summary for commit 36c0717CTS tests (Failed: 0/138443)
Ubuntu navi3x, SrdcvkUbuntu navi2x, Srdcvk |
This change will perform defined behavior for global buffer access in
the case of null descriptor or out-of-bound if the application need the
extended
robustnessAccess.NOTE: we use dword2 to check null descriptor.