[spirv] Add option to flatten array of resources. - #2397
Conversation
Ehsan (ehsannas)
changed the title
|
The failure is due to the missing spirv-tools update. I'll have to rebase this PR. |
Current SPIR-V code generation uses 1 binding number for an array of resources (e.g. an array of textures). However, the DX side uses one binding number per array element. The newly added '-fspv-flatten-resource-arrays' changes the SPIR-V backend behavior to use one binding number per array element, and uses spirv-opt to flatten the array. TODO: Add a test where the array is passed around. TODO: Test this works with steven's PR and proper results are produced.
1387080 to
c2e6298
Compare
Jaebaek Seo (jaebaek)
left a comment
Jaebaek Seo (jaebaek)
left a comment
There was a problem hiding this comment.
Mostly LGTM, but two minor issues.
Those issues are just my opinion but it is not critical and I will follow your decision.
|
|
||
| // Check whether the chunk of |n| binding numbers can be fitted at the | ||
| // very beginning of the list (start at binding 0 in the current set). | ||
| if ((*existingBindings.begin()) >= n) |
There was a problem hiding this comment.
This is based on the assumption that std::set::iterator always visits elements in sorted order.
I just quickly checked a discussion and it looks fine, but I am just worried what if it causes a problem in the future (e.g., other stl implementations?).
Maybe putting a comment here about the assumption would be helpful?
I am not 100% sure about this, just I will follow your opinion.
There was a problem hiding this comment.
Yes, the std::set iterator always iterates on a sorted basis. That's why we also have std::unordered_set, which only contains a set of things that are not necessarily sorted.
| if (existingBindings.empty()) | ||
| return 0; | ||
|
|
||
| // Check whether the chunk of |n| binding numbers can be fitted at the |
There was a problem hiding this comment.
I think the following code can avoid twice of iter != exisitingBindings.end() and curBinding = *iter, nextBinding = *iter:
// Check whether the chunk of |n| binding numbers can be fitted at the
// very beginning of the list (start at binding 0 in the current set).
uint32_t curBinding = *existingBindings.begin();
if (curBinding >= n)
return 0;
auto iter = std::next(existingBindings.begin());
while (iter != existingBindings.end()) {
// There exists a next binding number that is used. Check to see if the
// gap between current binding number and next binding number is large
// enough to accommodate |n|.
uint32_t nextBinding = *iter;
if (n <= nextBinding - curBinding - 1)
return curBinding + 1;
curBinding = nextBinding;
// Peek at the next binding that has already been used (if any).
++iter;
}
// |curBinding| was the last binding that was used in this set. The next
// chunk of |n| bindings can start at |curBinding|+1.
return curBinding + 1;
Notice that because of if (existingBindings.empty()) return 0; above we can assume that std::next(existingBindings.begin()) is not existingBindings.end().
Please let me know if I misunderstood something.
There was a problem hiding this comment.
you're right. I'll make the update.
3 participants
No description provided.