Skip to content
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

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

derek-gerstmann
Copy link
Contributor

Previous refactoring accidentally used the fully qualified var name rather than the categorized vulkan intrinsic name.

See also #8258.

accidentally used the fully qualified var name rather than the
categorized vulkan intrinsic name.
@derek-gerstmann
Copy link
Contributor Author

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

Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending green

@steven-johnson
Copy link
Contributor

Looks like we are getting a segfault in generator_aot_cleanup_on_error for vulkan

@derek-gerstmann
Copy link
Contributor Author

derek-gerstmann commented Jun 4, 2024

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 generator_aot_cleanup_on_error. This seems like memory corruption due to the custom halide alloc routine, which then clobbers the shader code somehow. Investigating some more ...

@derek-gerstmann
Copy link
Contributor Author

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 0D are replaced with OA). Strange!

@steven-johnson
Copy link
Contributor

There's a semi regular pattern of bytes that have been overwritten (all binary values of 0D are replaced with OA). Strange!

something is treating it as a text file/stream and trying to convert LF <-> CR... verify that all files are opened in binary mode?

@derek-gerstmann
Copy link
Contributor Author

There's a semi regular pattern of bytes that have been overwritten (all binary values of 0D are replaced with OA). Strange!

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 _gpu_source_kernels. So there shouldn't be any text translation occuring. Still digging.

@derek-gerstmann
Copy link
Contributor Author

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.

@derek-gerstmann derek-gerstmann merged commit 4b67712 into main Jun 5, 2024
7 checks passed
@steven-johnson
Copy link
Contributor

Should we backport this to the 17.x release?

steven-johnson added a commit that referenced this pull request Jun 6, 2024
* 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 <dgerstmann@adobe.com>
Co-authored-by: Steven Johnson <srj@google.com>
@derek-gerstmann
Copy link
Contributor Author

Yes … I think both the GPU Vars and CodeGen_C string formatting are in v17.x

steven-johnson added a commit that referenced this pull request Jun 24, 2024
* [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 <dgerstmann@adobe.com>
Co-authored-by: Steven Johnson <srj@google.com>

* 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 <dgerstmann@adobe.com>

* Backport fixes for Vulkan in src/runtime/internal for allocations.

---------

Co-authored-by: Derek Gerstmann <derek.gerstmann@gmail.com>
Co-authored-by: Derek Gerstmann <dgerstmann@adobe.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants