KhronosGroup / SPIRV-LLVM-Translator

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

Add support for multiple Intel cache controls on a single argument #2599

Closed MrSidims closed 3 months ago

MrSidims commented 3 months ago

In this case zero GEP will be created a single time, all the decorations will be attached to it.

aratajew commented 3 months ago

How will this implementation behave for the following LLVM IR:

%common_dummy_ptr_generated_by_user = getelementptr ptr addrspace(1), ptr addrspace(1) %ptr, i32 0

call void @read0(ptr addrspace(1) %common_dummy_ptr_generated_by_user), !spirv.DecorationCacheControlINTEL !0
call void @read1(ptr addrspace(1) %common_dummy_ptr_generated_by_user), !spirv.DecorationCacheControlINTEL !3

!0 = !{!1, !2}
!1 = !{i32 6442, i32 0, i32 3, i32 0}  ; LoadCacheControlINTEL, CacheLevel = 0, InvalidateAfterRead
!2 = !{i32 6442, i32 1, i32 1, i32 0}  ; LoadCacheControlINTEL, CacheLevel = 1, Cached
!3 = !{!4, !5}
!4 = !{i32 6442, i32 0, i32 0, i32 0}  ; LoadCacheControlINTEL, CacheLevel = 0, Uncached
!5 = !{i32 6442, i32 1, i32 0, i32 0}  ; LoadCacheControlINTEL, CacheLevel = 1, Uncached

Is my understanding correct that the SPIRVWriter will detect GEP0 generated by a user and therefore it'll try to assign all the decorations to the same pointer?

MrSidims commented 3 months ago

@aratajew Sorry, I incidentally edited your comment instead of mine, restore the original one.

Is my understanding correct that the SPIRVWriter will detect GEP0 generated by a user and therefore it'll try to assign all the decorations to the same pointer?

Correct, that is what would happen.

aratajew commented 3 months ago

So it seems incorrect. User expected to execute:

victor-eds commented 3 months ago

As a NIT, we could be using SmallPtrSet instead of std::vector, but I'm fine with current approach.

victor-eds commented 3 months ago

My approval is "grey" here, so you'll need a proper review to merge

MrSidims commented 3 months ago

All of the tests are unrelated and will be fixed in https://github.com/KhronosGroup/SPIRV-LLVM-Translator/pull/2602