-
Notifications
You must be signed in to change notification settings - Fork 74
[intel] 2Dblock runtime HW checks #5252
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
Conversation
|
Need to find a clean way to use __assert_fail in tritongen to llvm pass instead of llvm trap. |
6cf28c1 to
19da69b
Compare
e15c893 to
b549ca9
Compare
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.
Pull Request Overview
This PR implements runtime hardware checks for 2D block operations in the Intel backend by adding runtime assertion validation that verifies parameter constraints when the TRITON_INTEL_2DBLOCK_ASSERT environment variable is set.
Key changes include:
- Added LibCallEmitter utility class for generating library calls including printf and assert_fail functions
- Integrated runtime validation into 2D block load/store/prefetch operations with parameter constraint checking
- Added comprehensive test coverage for the new assertion functionality
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| third_party/intel/lib/Utils/LibCallEmitter.h | New header defining LibCallEmitter class for library call generation |
| third_party/intel/lib/Utils/LibCallEmitter.cpp | Implementation of LibCallEmitter with printf and assertFail methods |
| third_party/intel/lib/Utils/CMakeLists.txt | Added LibCallEmitter.cpp to build configuration |
| third_party/intel/lib/TritonIntelGPUToLLVM/TargetInfo.h | Updated to use LibCallEmitter instead of inline globals |
| third_party/intel/lib/TritonIntelGPUToLLVM/TargetInfo.cpp | Refactored to delegate library calls to LibCallEmitter |
| third_party/intel/lib/TritonGENToLLVM/TritonGENToLLVMPass.cpp | Added 2D block parameter validation with runtime checks |
| third_party/intel/include/TritonGENToLLVM/TritonGENToLLVMPass.h | Updated function signature to accept LibCallEmitter |
| test/TritonGEN/tritongen-2Dblockload-to-llvm-asserts.mlir | MLIR test verifying assert generation based on environment variable |
| python/test/unit/intel/test_block_load.py | Python test ensuring assertions trigger on invalid parameters |
| python/test/unit/intel/block_load_helper.py | Helper script for running assertion tests |
| include/triton/Tools/Sys/GetEnv.hpp | Added TRITON_INTEL_2DBLOCK_ASSERT to cache-invalidating environment variables |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
780edb8 to
feaabc0
Compare
| torch.testing.assert_close(result_tri, result_tor, atol=1e-2, rtol=1e-3) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("elem_size, width, height, pitch, x", |
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.
Runtime checks to see if asserts actually work.
|
|
||
| } // namespace | ||
|
|
||
| //===----------------------------------------------------------------------===// |
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.
I removed it because it seems to be unused and otherwise I'd have to somehow handle lifecycle of calls emitter in populate* method.
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.
Can you try to enable the env var temporarily to see if there is any unexpected failures in our testing?
feaabc0 to
ad80e6e
Compare
Yes, just added a commit that removes the check. |
5809361 to
0dede8e
Compare
|
@whitneywhtsang When I enabled it for all CI tests it failed in some minicore tests. I'd prefer to resolve it in follow-up issue and I also made a failing PR for reference: #5326 |
0dede8e to
8d0d554
Compare
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.
nice!
Solution for #5110