KhronosGroup / SPIRV-LLVM-Translator

A tool and a library for bi-directional translation between SPIR-V and LLVM IR
Other
464 stars 208 forks source link

Experiment with new form of SPIR-V friendly decoration representation #2566

Closed MrSidims closed 1 month ago

MrSidims commented 2 months ago

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.

MrSidims commented 2 months ago

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.

svenvh commented 2 months ago

spirv.Decorations metadata and annotation intrinsics have a drawback that they can be optimized out.

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.

aratajew commented 2 months ago

The support for attaching spirv.Decorations metadata to instructions in SPIR-V Friendly IR was added strictly for the purposes of supporting SPV_INTEL_cache_controls extension. I think it's worth to emphasize that the way the extension is designed is quite extraordinary. This is because it allows to decorate a pointer with cache controls decoration, but it only has any effect if the pointer is directly used by a memory-reading/memory-writing instruction:

If a memory-reading instruction uses the decorated pointer value as its input parameter, the decoration is a hint that the load operation should be performed with the specified cache semantics.

If a memory-writing or atomic instruction uses the decorated pointer value as its input parameter, the decoration is a hint that the store operation should be performed with the specified cache semantics.

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 spirv.Decorations metadata to be attached to a memory-accessing instruction:

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:

If a memory-reading instruction uses the decorated pointer value as its input parameter, the decoration is a hint that the load operation should be performed with the specified cache semantics.

If a memory-writing or atomic instruction uses the decorated pointer value as its input parameter, the decoration is a hint that the store operation should be performed with the specified cache semantics.

And most importantly, it is much easier to preserve it across LLVM's optimization pipeline than GEPs. It also aligns with !nontemporal representation in LLVM IR.

@MrSidims , @svenvh , @AlexeySachkov , @zuban32 what do you think?

MrSidims commented 2 months ago

@aratajew the problem is (and here I agree with Sven) that it also can disable optimizations. For example if we have a pointer to a small structure created by alloca with a sequence of load/stores. In a normal circumstances I'd expect such structure be replaced with i64 and put its value on register by SROA + mem2reg, but when we have an external call with side effects it won't happen. And placing such memory on the register would be much more beneficial then decorating a pointer with cache controls. Same is applicable to other optimizations, that can be more beneficial then preserving cache controls.

UPD my apologies I misread the comment, I agree that Andrzej's proposal is better

aratajew commented 2 months ago

The solution I've proposed above won't block promotion to registers.

zuban32 commented 2 months ago

The general idea sounds good to me. However it's not clear whether do we introduce a new kind of metadata (spirv.CacheControlLoadINTEL used in your example) or stick with original spirv.Decorations?

MrSidims commented 1 month ago

Closing in favor of https://github.com/KhronosGroup/SPIRV-LLVM-Translator/pull/2587