-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[vulkan] Fix Vulkan SIMT mappings for GPU loop vars. #8259
Conversation
accidentally used the fully qualified var name rather than the categorized vulkan intrinsic name.
Sadly, we missed this regression since we disabled the Vulkan backend from running on the build bots due to spurious errors when we were getting the last release out the door. With all the recent Vulkan fixes, perhaps we should reconsider turning them back on? @steven-johnson |
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 pending green
Looks like we are getting a segfault in generator_aot_cleanup_on_error for vulkan |
Tried to reproduce on a Linux VM, and OSX but neither seem to trigger this. I'll try native Linux ... Okay, was able to reproduce the error for |
Confirmed that the SPIRV binary that's created during CodeGen when the generator is run, does not match the SPIRV binary that's passed to the runtime during the execution of the test. There's a semi regular pattern of bytes that have been overwritten (all binary values of |
something is treating it as a text file/stream and trying to convert LF <-> CR... verify that all files are opened in binary mode? |
Ah, that seems likely. But I'm not sure where this is coming from ... the binary gets injected into a buffer that's exposed through the |
Aha! Found it ... the host-side CodeGen that invokes the GPU runtime uses CodeGen_C to encode the GPU kernel source into a formatted string. I'll disable this formatting for Vulkan to avoid the text translation. |
…nary SPIR-V needs to remain intact).
Should we backport this to the 17.x release? |
* Fix Vulkan SIMT mappings for GPU loop vars. Previous refactoring accidentally used the fully qualified var name rather than the categorized vulkan intrinsic name. * Avoid formatting the GPU kernel to a string for Vulkan (since it's binary SPIR-V needs to remain intact). --------- Co-authored-by: Derek Gerstmann <[email protected]> Co-authored-by: Steven Johnson <[email protected]>
Yes … I think both the GPU Vars and CodeGen_C string formatting are in v17.x |
* [vulkan] Fix Vulkan SIMT mappings for GPU loop vars. (#8259) * Fix Vulkan SIMT mappings for GPU loop vars. Previous refactoring accidentally used the fully qualified var name rather than the categorized vulkan intrinsic name. * Avoid formatting the GPU kernel to a string for Vulkan (since it's binary SPIR-V needs to remain intact). --------- Co-authored-by: Derek Gerstmann <[email protected]> Co-authored-by: Steven Johnson <[email protected]> * Update CodeGen_Vulkan_Dev.cpp * [vulkan] Add conform API methods to memory allocator to fix block allocations (#8130) * Add conform API methods to block and region allocator classes Override conform requests for Vulkan memory allocator Cleanup memory requirement constraints for Vulkan Add conform test cases to block_allocator runtime test. * Clang format/tidy pas * Fix unsigned int comparisons * Clang format pass * Fix other unsigned int comparisons * Fix mismatched template types for max() * Fix whitespace for clang format --------- Co-authored-by: Derek Gerstmann <[email protected]> * Backport fixes for Vulkan in src/runtime/internal for allocations. --------- Co-authored-by: Derek Gerstmann <[email protected]> Co-authored-by: Derek Gerstmann <[email protected]>
Previous refactoring accidentally used the fully qualified var name rather than the categorized vulkan intrinsic name.
See also #8258.