-
Notifications
You must be signed in to change notification settings - Fork 204
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
Experiment with new form of SPIR-V friendly decoration representation #2566
Conversation
spirv.Decorations metadata and annotation intrinsics have a drawback that they can be optimized out. This patch adds experimental approach of preserving decorations with an external function call with __spirv_Decoration prefix. To start with Intel Cache Controls decorations are being added. Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>
Unfortunately todays TSG was postponed. @svenvh may I ask you to look if justification for such patch seems legit to you? The code itself will be rewritten, so Regularize pass will have a single iteration over the functions in the module and some other refactoring will be done, so no need to review it now. |
std::vector<Instruction *> ToErase; | ||
for (auto FI = M->begin(), FE = M->end(); FI != FE;) { | ||
Function *F = &(*FI++); | ||
// For the prototype __translate_spirv_cache_control_*** is assumed to have |
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.
outdated name
Did you experience that in practice and have you considered trying harder to preserve the metadata somehow? No strong objections for going with external function calls, but one thing to consider: llvm is moving away from debuginfo intrinsics, because they're "slow, unwieldy and can confuse optimisation passes". So I'm slightly worried that adding intrinsics to convey metadata seems to be a move in the opposite direction again. |
The support for attaching
Even though the decoration is applied to a pointer, it is effectively not a pointer's property, but rather a property of a memory-accessing instruction. The extension has been designed this way, because pointer is the only intersection (common point) between all memory-accessing instructions in SPIRV. I think that it's worth to reiterate on the way SPIRV cache controls are represented in the SPIR-V Friendly IR. Current form of SPIR-V Friendly IR reflects the SPIRV representation 1 to 1, but as Dmitry mentioned it has a significant drawback that it is not resistant to LLVM optimizations. This is currently a critical issue that needs to be addressed as it significantly impacts/blocks our clients. Taking into account that SPIRV cache controls decorations have only effect if a decorated pointer is directly used by a memory-accessing instruction, we should think of it as an instruction's property, rather than pointer's property. Therefore, I would propose to move call spir_func void @_Z20__spirv_ocl_prefetchPU3AS1Kcm(ptr addrspace(1) %ptr, i64 2) !spirv.CacheControlLoadINTEL !0
...
call spir_func void @_Z20__spirv_ocl_prefetchPU3AS1Kcm(ptr addrspace(1) %ptr, i64 4) !spirv.CacheControlLoadINTEL !4
!0 = {!1, !2, !3}
!1 = {i32 0} ; {argument index that decoration applies to}
!2 = {i32 0, i32 1} ; {CacheLevel=0, Cached}
!3 = {i32 1, i32 1} ; {CacheLevel=1, Cached}
!4 = {!5, !6, !7}
!5 = {i32 0} ; {argument index that decoration applies to}
!6 = {i32 0, i32 0} ; {CacheLevel=0, Uncached}
!7 = {i32 1, i32 0} ; {CacheLevel=1, Uncached} I think that it much better reflects these two sentences from SPV_INTEL_cache_controls specification:
And most importantly, it is much easier to preserve it across LLVM's optimization pipeline than GEPs. It also aligns with @MrSidims , @svenvh , @AlexeySachkov , @zuban32 what do you think? |
@aratajew UPD my apologies I misread the comment, I agree that Andrzej's proposal is better |
The solution I've proposed above won't block promotion to registers. |
The general idea sounds good to me. However it's not clear whether do we introduce a new kind of metadata ( |
Closing in favor of #2587 |
spirv.Decorations metadata and annotation intrinsics have a drawback that they can be optimized out. This patch adds experimental approach of preserving decorations with an external function call with __spirv_Decoration prefix. To start with Intel Cache Controls decorations are being added.
SPIRVRegularizeLLVM pass will replace such call with it's argument aka value to decorate and add spirv.Decorations metadata to be recognized by the SPIRVWriter.
That is experimental approach, so we are not ready to document it yet.