-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ByteAddressBuffer support #248
Add ByteAddressBuffer support #248
Conversation
ca8e5d0
to
30efe84
Compare
30efe84
to
7ebf3ad
Compare
7ebf3ad
to
0217071
Compare
break; // arithmetic starts here | ||
} | ||
arithmetic_node_stack[arithmetic_count++] = p_next_node; | ||
if (arithmetic_count >= 8) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check be above this? Otherwise it will return if you have 8 operations when it could have succeeded (if you have exactly 8). I also don't understand why there is such a low cap on this, it seems rather inflexible. Would it be better to scan for how many first then allocate an array with a cap of like the size of the spir-v or something?
The problem is that SPIR-V reflection will completely fail in this case, not just that you won't get the offsets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so before it failed because I returned an error, not I return the same as "could not find" it it hits the cap
I guess there is no reason for a hard limit, it just I have yet to find a case where it is more then 2 here
Would it be better to scan for how many first
So the other issue is I do need "a cap" when doing this search anyways, there is no "correct" way for HLSL/Slang to flatten the ByteAddressBuffer
so in the future, if some strange pattern arises, I still need some arbitrary number to stop at incase I get in some strange situation and don't want to crash
I've tested the latest PR with our full shader corpus and can confirm it's working properly and producing proper ByteAddressBuffer offsets. Looks good to merge to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
attempt 2 of #236 (closes #234)
When
ByteAddressBuffer
are collapsed they start to look likeusing the
UserType
we can detect which variable is used, from here we can build up an offset by working out the math