Closed MrSidims closed 1 month ago
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: https://github.com/MrSidims/SPIRV-LLVM-Translator/commit/156fe23c403ba005efd6b5efea2f2d51169af89e
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.
@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.
Is there any chance to make this annotation more general than "spirv.DecorationCacheControlINTEL" in the nearest future
@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?
I think that it is reasonable for SPV_INTEL_cache_controls extension to have its own metadata spirv.DecorationCacheControlINTEL
in SPIR-V Friendly IR. This is because SPV_INTEL_cache_controls extension is kind of extraordinary as it allows to decorate a pointer, but it only has any effect if the decorated pointer is directly used by memory-reading/memory-writing instruction. So it actually should be treated as a property of an instruction not a pointer, but due to SPIRV limitations (OpStore cannot be decorated) the decoration is applied to a pointer. I've provided a more extensive explanation about this here.
But if we find that this kind of representation can also be useful for any other extension, I'm not against making it more generic.
@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: