-
Notifications
You must be signed in to change notification settings - Fork 44
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
[TritonGEN] Add triton_gen.cache_controls
operation
#1087
Conversation
|
Instead of expressing cache control at the pointer, the new proposal is to express at the memory operation (KhronosGroup/SPIRV-LLVM-Translator#2587). |
Add `triton_gen.cache_control` operation to represent [SPV_INTEL_cache_controls](https://htmlpreview.github.io/?https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_cache_controls.html) decorations in MLIR. This operation does not convert to any operation in a different dialect, as it supports translation straight to LLVM IR as metadata. `triton-translate` is a new tool used to test this translation. Signed-off-by: Victor Perez <victor.perez@codeplay.com>
81bf974
to
49719dd
Compare
Updated implementation. This is ready to review now. |
Given that the new design has cache control on the memory operation instead of the pointer, would it make sense for TritonGEN to add an attribute instead of a new operation that express the cache control on the pointer? |
The cache control is a property of the load/store operations rather than the ptr used by those operations. So I think the attribute is a better way to go. |
My fear is that MLIR transform passes may drop these attributes, as these are not guaranteed to be preserved. That's why I went with this design. |
triton_gen.cache_control
operationtriton_gen.cache_controls
operation
It is the same concern for metadata on memory operations too. Do you think there is a high chance that attributes would be dropped in practice? |
That will depend on how late in the pipeline these are added. The only way we can make sure these are added is if the cache control information is to be added as part of a conversion to LLVM dialect pattern. Otherwise, even if we have an operation |
Got it, however the chance is small because we aren't running any transformations on the TritonGEN code |
Yeah, I was thinking more on:
If the cache control is to be set only when creating the corresponding |
Are we fine with current approach or do we wanna switch to using discardable attributes in operations? @whitneywhtsang @etiotto If this cache info is to be inserted as part of the conversion to LLVM dialect, I think we can go with the attribute design. |
My preference is attributes in operations. |
The 2D block load operation in the TritonGen dialect already has an attribute for the cache control. When we lower that operation into a call we cannot pass the cache control as an argument because the "builtin" call we need to generate does not have that argument. Instead of taking the cache control as an argument we need to generate metadata and attach it to the function call. |
Signed-off-by: Victor Perez <victor.perez@codeplay.com>
With the changes I've just pushed, that'd involve attaching an attribute to the call operation as in the examples. |
Add
triton_gen.cache_controls
operation to represent SPV_INTEL_cache_controls decorations in MLIR.This operation does not convert to any operation in a different dialect, as it supports translation straight to LLVM IR as metadata.