-
Notifications
You must be signed in to change notification settings - Fork 246
Re-implement Intel cache controls decoration #2587
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
Re-implement Intel cache controls decoration #2587
Conversation
If somebody prefers another implementation (2nd mentioned for example) here is a draft to express opinion if unprivating a method in the scavenger is OK: MrSidims@156fe23 |
The patch adds CacheControls(Load/Store)INTEL decorations representation as metadata placed on memory accessing instruction with the following form: !spirv.DecorationCacheControlINTEL !X !X = !{i32 %decoration_kind%, i32 %level%, i32 %control%, i32 %operand of the instruction to decorate%} Also patch creates a dummy GEP accessing pointer operand of the instruction to perform SSA copy of the pointer and attaches !spirv.Decorations metadata to the GEP. Few notes about this implementation and other options. It is sub-optimal as it requires iteration over all instructions in the module. Alternatives are: 1. In SPIRVWriter during transDecorations rewrite already translated instruction (lets say load) with the new one, but with GEP operand. But while the translator provides API to erase instructions and rewriting SPIRVBasicBlocks, yet unfortunatelly it's not really usable outside of of SPIRVModule context; 2. In SPIRVWriter during transValueWithoutDecoration add special handling of the instructions with spirv.DecorationCacheControlINTEL metadata. And it would work with one exception - it also would require to unprivate some methods of SPIRVScavenger to create GEP with the correct type, which I'm not sure if it's a good idea. Note, in this implementation in the worst case scenario SPIRVScavenger would create a bitcast later during translation. Signed-off-by: Sidorov, Dmitry <[email protected]>
6f47ea7
to
01912d6
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.
Overall, the change LGTM. Should we document it in the SPIR-V Friendly IR doc?
In general - yes, but I would like to see if our experiment goes well and if any other changes are required. For example the metadata can be generalized (if we find it's useful in other cases) to spirv.DecorationsInstructionPtrOp, but for now I'm not keen to spell it out this way. |
Signed-off-by: Sidorov, Dmitry <[email protected]>
@MrSidims Is there any chance to make this annotation more general than "spirv.DecorationCacheControlINTEL" in the nearest future? The PR itself looks good to me, and it's better than another option, in my opinion, but my concern is that while "spirv.Decorations" and "spirv.ParameterDecorations" looks natural and neatly, as a single entry point/bridge between LLVM IR/SPIR-V necessities, a newly introduced "spirv.DecorationCacheControlINTEL" is so evidently a particular case brought on a higher interface level between LLVM IR and SPIR-V that it doesn't seem elegant and feels not quite right. I don't have any valuable comments wrt. the code of the PR, it's just the approach with an exposure of a corner case as something normal makes me wonder whether it should be generalized before applying. |
test/extensions/INTEL/SPV_INTEL_cache_controls/basic_load_store_arg_dec.ll
Outdated
Show resolved
Hide resolved
@VyacheslavLevytskyy yes, it's named like this for now for experiments. If we find it's useful in other cases it can be renamed. Right now it follows notation from spirv.Decorations metadata, so the (possible) switch would be painless. @svenvh would you find such new format useful for your extensions? |
Signed-off-by: Sidorov, Dmitry <[email protected]>
I think that it is reasonable for SPV_INTEL_cache_controls extension to have its own metadata |
@aratajew Indeed, however, there is one more argument towards making this a general case eventually. After experiments are over and we'll get the final version of this feature, we'd need to add it upstream, to SPIR-V Backend in llvm.org. |
The patch adds CacheControls(Load/Store)INTEL decorations representation as metadata placed on memory accessing instruction with the following form: !spirv.DecorationCacheControlINTEL !X
!X = !{i32 %decoration_kind%, i32 %level%, i32 %control%,
i32 %operand of the instruction to decorate%}
It's being done this way as it's less likely for the metadata attached to function calls or memory instructions being optimized out than in a case when it's attached to pointer in LLVM IR.
Also patch creates a dummy GEP accessing pointer operand of the instruction to perform SSA copy of the pointer and attaches !spirv.Decorations metadata to the GEP, so several memory access instructions with different cache locality would use different decorated copies of the same pointer.
Few notes about this implementation and other options. It is sub-optimal as it requires iteration over all instructions in the module. Alternatives are: