Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

MrSidims
Copy link
Contributor

@MrSidims MrSidims commented May 16, 2024

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.

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>
@MrSidims
Copy link
Contributor Author

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outdated name

@svenvh
Copy link
Member

svenvh commented May 16, 2024

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
Copy link
Contributor

aratajew commented May 17, 2024

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
Copy link
Contributor Author

MrSidims commented May 17, 2024

@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
Copy link
Contributor

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

@zuban32
Copy link
Contributor

zuban32 commented May 18, 2024

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
Copy link
Contributor Author

Closing in favor of #2587

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants