How to deal with opaque pointers in the translator

[MrSidims]

for reference: https://llvm.org/docs/OpaquePointers.html#migration-instructions
We have discussed some high-level plan about what to do in the translator. This issue is reflecting this plan leaving the field to discuss the details.

LLVM community is moving towards typeless pointers. There are several things to do in the translator to make sure typeless pointers are translatable:
0. Get rid of LLVM pointer-element-type dependent API in the translator code base, for example getElementType itself;

1. Represent typeless pointers somehow. Currently we don't have any extension for this and even if we had - still we would need to translate such LLVM IR to SPIR-V and backwards. As an option we can create pointers to a magically named opaque structure in SPIR-V. Note, that it would require to add extra casts to/from this new type for the pointer users.

2. During reverse translation there is no good way to differ this opaque structure pointer from the true typeless pointer. So to make sure, that new typesless pointer is being emitted by SPIR-V consumer we probably need to introduce an option that forces any pointer type to become opaque. This option is also enforcing the translator to eliminate casts that were inserted on the previous step (or that were presenting in the LLVM IR before). If the option is not passed during the reverse translation (default behavior at least for the first time), the translator should emit LLVM IR as there are no typeless pointers at all (aka emit pointers to opaque structure).

3. We actually need to have opaque/typeless pointer extension and make it properly working with the mentioned SPIR-V consumer option.

At minimum we would need to translate the following IR snippet, I assume the translation flow (without an extension) should be looking like:
LLVM IR before the translator:

define spir_kernel void @test(ptr addrspace(1) %p) {
  store i32 0, ptr %p
  %v = load i64, ptr %p
  ret void
}

Generated SPIR-V:

5 EntryPoint 6 19 "test"
3 Source 0 0
6 Name 3 "struct.Magic"
4 Name 6 "test"
3 Name 7 "p"
4 Name 8 "entry"
3 Name 18 "v"
3 Name 20 "p"
6 Decorate 6 LinkageAttributes "test" Export
4 TypeInt 9 32 0
4 TypeInt 15 64 0
4 Constant 9 14 0
2 TypeVoid 2
6 TypeOpaque 3 "struct.Magic"
4 TypePointer 4 5 3
4 TypeFunction 5 2 4
4 TypePointer 10 5 9
4 TypePointer 12 8 9
4 TypePointer 16 8 15


5 Function 2 6 0 5
3 FunctionParameter 4 7

2 Label 8
4 Bitcast 10 11 7
4 PtrCastToGeneric 12 13 11
5 Store 13 14 2 4
4 Bitcast 16 17 13
6 Load 15 18 17 2 8
1 Return

1 FunctionEnd

5 Function 2 19 0 5
3 FunctionParameter 4 20

2 Label 21
5 FunctionCall 2 22 6 20
1 Return

1 FunctionEnd

LLVM IR after the translator (with --force-opaque-pointers option):

define spir_kernel void @test(ptr addrspace(1) %p) {
  store i32 0, ptr %p
  %v = load i64, ptr %p
  ret void
}

LLVM IR after the translator (without --force-opaque-pointers option):

define spir_kernel void @test(%struct.Magic addrspace(1)* %p) {
entry:
  %0 = bitcast %struct.Magic addrspace(1)* %p to i32 addrspace(1)*
  %1 = addrspacecast i32 addrspace(1)* %0 to i32 addrspace(4)*
  store i32 0, i32 addrspace(4)* %1, align 4
  %2 = bitcast i32 addrspace(4)* %1 to i64 addrspace(4)*
  %v = load i64, i64 addrspace(4)* %2, align 8
  ret void
}

[MrSidims]

As an option we can create pointers to magically named opaque structures in SPIR-V.

Or actually, we can just emit i8* in SPIR-V.

[MrSidims]

Tagging @AlexeySachkov @vmaksimo @bader @romanovvlad @svenvh @AnastasiaStulova @StuartDBrady

[MrSidims]

Possible problem with the proposed design (and opaque pointers in general): would clSetKernelArg and other kernel arg APIs work correctly with the opaque pointers?

[AlexeySachkov]

Represent typeless pointers somehow. Currently we don't have any extension for this and even if we had - still we would need to translate such LLVM IR to SPIR-V and backwards. As an option we can create pointers to a magically named opaque structure in SPIR-V. Note, that it would require to add extra casts to/from this new type for the pointer users.

I suggest that we proceed with phase 0 before deciding on the next steps. The reason for that is that generating magic opaque structures is an extra work we will have to do and presence of such structures could confuse backends consuming SPIR-V: not that functionality will be affected, but performance might be impacting due to analysis passes incorrectly handle extra opaque structures.

Hopefully, we will be able to deduce most of pointer types from neighbor instructions as suggested in migration instructions and generate SPIR-V which is the same as we generate now without opaque pointers in input.

[jcranmer-intel]

I've got some local patches that start with the transition to opaque pointers. The first of these is already a PR (#1463), and I'm holding off on posting the others more because Github works poorly with patches-that-depend-on-others. For now, I've focused on part 0, getting rid of getPointerElementType, and the following is an inventory of what needs to be done to do this:

The first set of changes is the "trivial" set of changes--the cases where you have to look slightly further afield to get the type of pointer (for example, getValueType for load/store instructions) rather than querying the pointer type. One alternate source of information here is the sret type parameter, which led me to the discovery that it was being generated incorrectly and hence PR #1463. A patch for these changes will be forthcoming.

The second set of changes is getting SPIRVReader to not rely on Type::getPointerElementType. I've got this working by using SPIRVType::getPointerElementType instead. A patch here will also be forthcoming, but it will take a little longer to disentangle things.

This leaves two residual sets of uses of getPointerElementType that are rather more challenging to convert. The first of these uses is in worrying about how to generate typed pointer SPIR-V from opaque pointer LLVM IR. The approach of mapping these types to a uniform i8* or pointer to magic opaque-pointer struct as contemplated above I suspect isn't going to actually produce working code for a large fraction of programs, given the other set I mention here. Deducing the pointer types from neighbor instructions and inserting extra bitcasts as necessary, as suggested in the previous comment, is likely to be the better path forward, but that is obviously going to require more work.

The more worrying set of uses is the queries to the struct name of the type returned by getPointerElementType (see most of the calls in lib/SPIRV/SPIRVUtil.cpp). There's a couple of effectively intrinsic-processing stuff (from what I can tell) that disambiguates processing based on the type names of these structs, and I worry that keeping struct names for pointer types in the output SPIR-V will be necessary for the actual subsequent device compiler code.

[kpet]

@alan-baker You might find this discussion interesting.

[svenvh]

The first of these uses is in worrying about how to generate typed pointer SPIR-V from opaque pointer LLVM IR. [...] Deducing the pointer types from neighbor instructions and inserting extra bitcasts as necessary, as suggested in the previous comment, is likely to be the better path forward, but that is obviously going to require more work.

Agreed; I think it'll be least disruptive for consumers of translator output if we aim to generate the same SPIR-V with and without opaque pointers. That will require some nontrivial effort on the translator side, but I think it's the best way forward.

There's a couple of effectively intrinsic-processing stuff (from what I can tell) that disambiguates processing based on the type names of these structs, and I worry that keeping struct names for pointer types in the output SPIR-V will be necessary for the actual subsequent device compiler code.

When you say "output SPIR-V" did you mean to say "output LLVM IR"? This is a valid concern, though the type names might also be available in the mangled name in most cases. That will require updating device compilers, but it might be inevitable when LLVM drops support for nonopaque pointers?

[jcranmer-intel]

I figured I'd give a not-so-brief status update of where I am in opaque pointer translation.

In my local workspace, I'm doing to 18 calls to getPointerElementType, which is about a third of where it was when I started. There's two big patches to achieve this level that I haven't uploaded yet because they're not close enough to finish:

• Patch the first solves a lot of opaque struct name queries by constructing a getParameterTypes query method that returns the StructType pointed to by each of its pointer arguments. This method is for now implemented using getPointerElementType, but the intent is to get this information by demangling. Additionally, this fixes a lot of helper methods to query a struct type for its name directly instead of querying the pointer element type for its struct name.
• Patch the second is the heavy lifting of SPIRVWriter to not use getPointerElementType. I have a rough plan of action that seems to be working well enough so far:
  • Add a type scavenger that finds the pointee type of all pointer typed-values in the program. If not all the uses of a value agree on the same pointee type, extra bitcasts will be added so that the SPIRVWriter knows where to insert them.
  • Add a transPointerType method that is roughly equivalent to today's transType(T->getPointerElementType(), T->getPointerAddressSpace()). Where the pointer type is known, this is used instead--it helps a lot with pointer-to-opaque-struct-are-really-special-types issues
  • Add a transScavengedType(Value *V) that will scavenge the appropriate type for a value based on its uses. Note that this includes function arguments, and my plan is to use this to scavenge function types as well, although actual function pointers may be somewhat more complicated.
  • Named struct types will get the types of any included pointee types from the type scavenger.
  • After this point, I don't have a firm plan. My hope is that the residuum of calls to transType that involve pointer types can be handled with real pointee types in non-opaque mode, or a generic pointer in opaque mode (effectively, this means you'd also not see nested pointer types). But I have not done anywhere near enough legwork to confirm this--I'm still working on rerouting calls to transType for pointer types.

Absent these patches, there are still several calls to getPointerElementType that I'm somewhat stuck on:

• https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/lib/SPIRV/SPIRVLowerSPIRBlocks.cpp#L287-L317 this is probably easy, but there's no exercise of this code in the test suite, so I can't confirm what the expected result looks like.
• There are a couple of opaque struct name queries that may be done on calls to builtins that aren't mangled (I don't remember which one that was offhand, sorry).
• Sampler and pipe initializers rely on a constant bitcast between different opaque structs. I don't have any path forward worked out for handling global variables and initializers, including this call.
• SPIRVRegularizeLLVMBase::adaptStructTypes does a lot of name remangling that I'm not fully certain of the significance of, nor how hard it is to do this without pointer element types.
• SPIRVLowerBitCastToNonStandardType handles non-standard vector types. It's probably possible to make this pass work without querying pointer element types, but it's also a nontrivial refactoring since it starts by looking for these types using bitcast instructions.

[YetAnotherCompilerEngineer]

Add a transPointerType method

I like it!

https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/lib/SPIRV/SPIRVLowerSPIRBlocks.cpp#L287-L317 this is probably easy, but there's no exercise of this code in the test suite, so I can't confirm what the expected result looks like.

Probably the following test addresses this code: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/test/transcoding/global_block.cl

SPIRVRegularizeLLVMBase::adaptStructTypes does a lot of name remangling that I'm not fully certain of the significance of, nor how hard it is to do this without pointer element types.

It actually does the following:

1. Find JointMatrixINTEL structure type (it's a structure with a pointer to multidimentional array inside);
2. Process pointer to this multidimentional array. Each dimention is used to store number of Rows, Columns etc.
3. Base type of the matrix is the same as a base type of the array.

I guess, we can go to https://github.com/intel/llvm/blob/sycl/sycl/include/CL/__spirv/spirv_types.hpp#L135 and remove the pointer, though it will require some code adjustments and actually doing some initialization of the structure, which is a bit undesired. But from the top of my head I don't see a better way.

Side note for it, https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/lib/SPIRV/SPIRVRegularizeLLVM.cpp#L459 is a wrong describtion, recently there was a 'Use' parameter added to the Matrix type layout, hence the comment should be updated accordingly.

[jcranmer-intel]

https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/lib/SPIRV/SPIRVLowerSPIRBlocks.cpp#L287-L317 this is probably easy, but there's no exercise of this code in the test suite, so I can't confirm what the expected result looks like.

Probably the following test addresses this code: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/test/transcoding/global_block.cl

It doesn't actually reach that method--I've dropped an assert(false); to check the context, and none of the tests fail. So I've done some more spelunking to see what's going on.

This pass appears to do nothing if there are no calls to spir_block_bind (as added by 1777597), which is the SPIR2 block API. At no point in the history of the test suite do I see any LLVM IR that contains a call to this function. The existing method addBlockBind can add a call to this method, but it seems the sole caller of this site was removed by 5d0e281, and was replaced by lowering to a different API for representing blocks.

So I think this code is dead and has been dead for several years. Indeed, if I disable the pass entirely and run the test suite, all of the tests pass. I'd like to propose removing this code entirely if it's truly dead, but I'm not savvy enough about how this code is used to say for certain.

---

Outside of that case, my local workspace is down to 4 calls to getPointerElementType. Two of these are crutch calls to keep tests passing while I work on the two big patches mentioned above. The third is the adaptStructTypes, which I haven't looked at in detail. The final one is this code: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/lib/SPIRV/SPIRVUtil.cpp#L1081-L1225, which is mangling names and I'm feeling pessimistic about how easy it is to convert.

I've got two PRs open right now to fix up, and probably another two or three PRs that aren't going to be rolled up into the big PRs.

[illwieckz]

Now that LLVM turned on opaque pointers by default ( llvm/llvm-project@702d5de ), llvm-spirv doesn't work anymore.

I first reported it on LLVM side:

• Crash when running pass 'Adapt OCL types for SPIR-V' on module 'builtins.link.spirv64-mesa3d-.bc' llvm/llvm-project#55121

but it looks like that's llvm-spirv issue.

Here is the kind of backtrace I get when building latest libclc, using latest llvm-spirv compiled against latest llvm.

llvm-spirv: include/llvm/IR/Type.h:384: llvm::Type *llvm::Type::getNonOpaquePointerElementType() const: Assertion `NumContainedTys && "Attempting to get element type of opaque pointer"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: bin/llvm-spirv --spirv-max-version=1.1 -o spirv64-mesa3d-.spv builtins.link.spirv64-mesa3d-.bc
1.	Running pass 'Adapt OCL types for SPIR-V' on module 'builtins.link.spirv64-mesa3d-.bc'.
Build libclc: [ 99%] Built target opt.spirv-mesa3d-.bc
Build libclc: [ 99%] Built target prepare-clspv--.bc
Build libclc: [ 99%] Built target prepare-clspv64--.bc
 #0 0x00007f74b67c97e2 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) llvm-project/llvm/lib/Support/Unix/Signals.inc:565:22
 #1 0x00007f74b67c98a9 PrintStackTraceSignalHandler(void*) llvm-project/llvm/lib/Support/Unix/Signals.inc:632:1
 #2 0x00007f74b67c7454 llvm::sys::RunSignalHandlers() llvm-project/llvm/lib/Support/Signals.cpp:103:20
 #3 0x00007f74b67c9113 SignalHandler(int) llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1
 #4 0x00007f74b5011520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #5 0x00007f74b5065828 __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #6 0x00007f74b5065828 __pthread_kill_internal ./nptl/pthread_kill.c:80:10
 #7 0x00007f74b5065828 pthread_kill ./nptl/pthread_kill.c:91:10
 #8 0x00007f74b5011476 gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #9 0x00007f74b4ff77b7 abort ./stdlib/abort.c:81:7
#10 0x00007f74b4ff76db get_sysdep_segment_value ./intl/loadmsgcat.c:509:8
#11 0x00007f74b4ff76db _nl_load_domain ./intl/loadmsgcat.c:970:34
#12 0x00007f74b5008e26 (/lib/x86_64-linux-gnu/libc.so.6+0x39e26)
#13 0x000055f6364f4e97 llvm::Type::getNonOpaquePointerElementType() const (bin/llvm-spirv+0x10be97)
#14 0x000055f636594c65 llvm::Type::getPointerElementType() const (bin/llvm-spirv+0x1abc65)
#15 0x000055f6365b91bd SPIRV::isPointerToOpaqueStructType(llvm::Type*) (bin/llvm-spirv+0x1d01bd)
#16 0x000055f63675e93d SPIRV::OCLTypeToSPIRVBase::adaptFunctionArguments(llvm::Function*) (bin/llvm-spirv+0x37593d)
#17 0x000055f63675e3c3 SPIRV::OCLTypeToSPIRVBase::runOCLTypeToSPIRV(llvm::Module&) (bin/llvm-spirv+0x3753c3)
#18 0x000055f63675e201 SPIRV::OCLTypeToSPIRVLegacy::runOnModule(llvm::Module&) (bin/llvm-spirv+0x375201)
#19 0x00007f74b6abc94d (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1545:20
#20 0x00007f74b6ab767a llvm::legacy::PassManagerImpl::run(llvm::Module&) llvm-project/llvm/lib/IR/LegacyPassManager.cpp:535:13
#21 0x00007f74b6abd25f llvm::legacy::PassManager::run(llvm::Module&) llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1673:1
#22 0x000055f63660aba2 llvm::writeSpirv(llvm::Module*, SPIRV::TranslatorOpts const&, std::ostream&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (bin/llvm-spirv+0x221ba2)
#23 0x000055f6364c1db5 convertLLVMToSPIRV(SPIRV::TranslatorOpts const&) llvm-spirv.cpp:0:0
#24 0x000055f6364bf942 main (bin/llvm-spirv+0xd6942)
#25 0x00007f74b4ff8fd0 __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#26 0x00007f74b4ff907d call_init ./csu/../csu/libc-start.c:128:20
#27 0x00007f74b4ff907d __libc_start_main ./csu/../csu/libc-start.c:379:5
#28 0x000055f6364bc5e5 _start (bin/llvm-spirv+0xd35e5)

[illwieckz]

Note: in contradiction to the documentation, building clang with -DCLANG_ENABLE_OPAQUE_POINTERS=OFF does not provide the previous behaviour (the exact same error occurs), see:

• CLANG_ENABLE_OPAQUE_POINTERS=OFF does not prevent Attempting to get element type of opaque pointer assertion llvm/llvm-project#55366

[alan-baker]

@MrSidims

Possible problem with the proposed design (and opaque pointers in general): would clSetKernelArg and other kernel arg APIs work correctly with the opaque pointers?

Have you considered a change to clang to set the ElementType attribute pointer kernel arguments? The attribute is described as being used with intrinsics which require the information, but it seems like it would fit in with the general design of maintaining the types wherever they are needed for the ABI.

[jcranmer-intel]

Have you considered a change to clang to set the ElementType attribute pointer kernel arguments? The attribute is described as being used with intrinsics which require the information, but it seems like it would fit in with the general design of maintaining the types wherever they are needed for the ABI.

Setting elementtype requires the function to itself be an LLVM intrinsic; since OpenCL and SPIR-V builtins are not LLVM intrinsics, this can't be used.

[alan-baker]

I just looked the verifier and you're right, but I guess that raises the question should there be an OpenCL attribute basically like element type for kernel arguments?

[jcranmer-intel]

Clang already creates metadata for OpenCL kernel argument types that contains the (source-level) type information for each parameter.

[YetAnotherCompilerEngineer]

There are such kernel_arg metadata, though I'm not sure, wether they are 'standart'. Currently the translator hadles them, transforming into OpString instruction and translates this string backwards only if the appropriate option is passed among -r option, see #1109 .

[bader]

There are such kernel_arg metadata, though I'm not sure, wether they are 'standart'. Currently the translator hadles them, transforming into OpString instruction and translates this string backwards only if the appropriate option is passed among -r option, see #1109 .

This metadata is emitted by OpenCL front-end only and it's optional. I think it's not emitted by DPC++ compiler.

---

I don't quite get what is the plan for the code example like this (https://godbolt.org/z/WzYYhTYe3):

; Function Attrs: convergent norecurse nounwind
define dso_local spir_func void @sample_func_read(ptr addrspace(1) noundef %0, ptr addrspace(1) %1, ptr addrspace(2) %2, <2 x float> noundef %3, <2 x float> noundef %4, <2 x float> noundef %5) local_unnamed_addr #0 {
  tail call spir_func void @my_lib_func(ptr addrspace(1) noundef %0, ptr addrspace(1) %1, ptr addrspace(2) %2, <2 x float> noundef %3, <2 x float> noundef %4, <2 x float> noundef %5) #2
  ret void
}

; Function Attrs: convergent
declare dso_local spir_func void @my_lib_func(ptr addrspace(1) noundef, ptr addrspace(1), ptr addrspace(2), <2 x float> noundef, <2 x float> noundef, <2 x float> noundef) local_unnamed_addr #1

This example doesn't use neither OpenCL built-in function calls nor kernel function definitions which might be used to recover SPIR-V type information from LLVM IR code.
Is the idea to declare a pointer to some specific type and bitcast it to OpTypeImage/OpTypeSampler to satisfy SPIR-V validation rules?

---

One of the goals of SPIR-V format states following: "Map easily to other intermediate languages". I think LLVM IR was mentioned there as one of the intermediate languages at some point. Today OpenCL front-end represents all OpenCL/SPIR-V specific types (e.g. sampler, image, pipe, etc.) as type-less pointer, so it doesn't seem to be easy to map LLVM IR to SPIR-V anymore. :(
To simplify the translation we should aim to preserve type information in LLVM, so that it can be reliably and relatively easy to recover (e.g. w/o using data flow analysis). We can also approach it from the other side and relax type requirements for SPIR-V instructions operands i.e. allow using type-less pointers for some instructions.

[jcranmer-intel]

This example doesn't use neither OpenCL built-in function calls nor kernel function definitions which might be used to recover SPIR-V type information from LLVM IR code. Is the idea to declare a pointer to some specific type and bitcast it to OpTypeImage/OpTypeSampler to satisfy SPIR-V validation rules?

My current approach is to back everything off to i8* if I can't deduce the type of the pointer, with bitcasts to specific types as necessary.

Today OpenCL front-end represents all OpenCL/SPIR-V specific types (e.g. sampler, image, pipe, etc.) as type-less pointer, so it doesn't seem to be easy to map LLVM IR to SPIR-V anymore. :( To simplify the translation we should aim to preserve type information in LLVM, so that it can be reliably and relatively easy to recover (e.g. w/o using data flow analysis).

I don't have much personal insight into the needs of the ultimate OpenCL backends with respect to these opaque types. However, the general trend in LLVM optimization passes is that retaining type information is usually more work than it is worth (beyond opaque pointer transition, there is a mild push towards converting geps to i8 offsets instead of retaining type information there). If these types only need to be distinguished at call sites to builtins, then something along the lines of #1477 is sufficient. If reasoning needs to happen more distantly from those call sites, then it is probably advisable to change their representation in LLVM: things are only going to get worse.

We can also approach it from the other side and relax type requirements for SPIR-V instructions operands i.e. allow using type-less pointers for some instructions.

At my current stage of SPIRVWriter work, I am running into some issues getting these types to validate properly in cases like PHI nodes or select statements. Nothing so far that isn't fixable, but I'm not finding an easy solution where I can be satisfied that the translation will be correct.

[bader]

This example doesn't use neither OpenCL built-in function calls nor kernel function definitions which might be used to recover SPIR-V type information from LLVM IR code. Is the idea to declare a pointer to some specific type and bitcast it to OpTypeImage/OpTypeSampler to satisfy SPIR-V validation rules?

My current approach is to back everything off to i8* if I can't deduce the type of the pointer, with bitcasts to specific types as necessary.

SPIR-V bitcast doesn't allow casting pointers to some types (e.g. OpTypeSampler) and I don't see any other standard instructions allowing such conversion. If I understand @MrSidims proposal in the issue description correctly, he suggests legalizing casts between type-less pointers and SPIR-V types with new extension to the spec. Am I right? Has anyone started working on the extension proposal?

@iliya-diyachkov, @michalpaszkowski, do you have a plan for dealing with SPIR-V types in SPIR-V back-end?

[asudarsa]

@bader, There is an ongoing effort to update SPIR-V extensions and accommodate untyped pointers. Here is a link to the merge request from @alan-baker: https://gitlab.khronos.org/spirv/spirv-extensions/-/merge_requests/202. Once the proposal is approved, I think we can start working on the implementation?
Thanks

[iliya-diyachkov]

@bader no special plan yet. Currently we are just following the direction in which the translator goes.
We have already implemented support for opaque pointers using i8*. We also expect the specification extension for opaque pointers, and will support type hints as metadata if the frontend attaches them.

[bader]

@asudarsa, thanks for the pointer. I looked through the document and it solves only part of the problem. This extension helps with translation types which are represented as a typed pointer in SPIR-V. The case I'm concerned about is SPIR-V types, which are not pointers, but still represented as a typed pointers in LLVM IR (e.g. OpTypeSampler is a pointer to opaque structure type with fixed name). SPIR-V translator is not able to recover a SPIR-V type if pointer type information is not present. The code snippet from this comment shows an example of LLVM IR with no type information in metadata or mangled function name.

and will support type hints as metadata if the frontend attaches them.

@iliya-diyachkov, does it mean clang is going to emit additional type information we can use to recover SPIR-V types like samplers/images/etc. for non-kernel functions as well?

[iliya-diyachkov]

does it mean clang is going to emit additional type information we can use to recover SPIR-V types like samplers/images/etc. for #1444 (comment) as well?

@bader I have not discussed it with clang guys, but I think it can be implemented this way.

[iliya-diyachkov]

@bader After talking to clang guys I suppose it's easy to make a patch which also hints arguments of non-kernel functions like this (for your example):

...
; Function Attrs: convergent norecurse nounwind
define dso_local spir_func void @sample_func_read(ptr addrspace(1) noundef %results, ptr addrspace(1) %image, ptr addrspace(2) %imageSampler, <2 x float> noundef %coord, <2 x float> noundef %dx, <2 x float> noundef %dy) local_unnamed_addr #0 !kernel_arg_addr_space !4 !kernel_arg_access_qual !5 !kernel_arg_type !6 !kernel_arg_base_type !7 !kernel_arg_type_qual !8 {
entry:
  tail call spir_func void @my_lib_func(ptr addrspace(1) noundef %results, ptr addrspace(1) %image, ptr addrspace(2) %imageSampler, <2 x float> noundef %coord, <2 x float> noundef %dx, <2 x float> noundef %dy) #2
  ret void
}

; Function Attrs: convergent
declare dso_local spir_func void @my_lib_func(ptr addrspace(1) noundef, ptr addrspace(1), ptr addrspace(2), <2 x float> noundef, <2 x float> noundef, <2 x float> noundef) local_unnamed_addr #1

attributes #0 = { convergent norecurse nounwind "frame-pointer"="all" "min-legal-vector-width"="64" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
attributes #1 = { convergent "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
attributes #2 = { convergent nounwind }

!llvm.module.flags = !{!0, !1}
!opencl.ocl.version = !{!2}
!opencl.spir.version = !{!2}
!llvm.ident = !{!3}

!0 = !{i32 1, !"wchar_size", i32 4}
!1 = !{i32 7, !"frame-pointer", i32 2}
!2 = !{i32 2, i32 0}
!3 = !{!"clang version 15.0.0 (/storage/home/idyachkov/spirv/llvm-project/clang 9a3144d078389c19b269b8dd94b9f5306754c039)"}
!4 = !{i32 1, i32 1, i32 0, i32 0, i32 0, i32 0}
!5 = !{!"none", !"read_only", !"none", !"none", !"none", !"none"}
!6 = !{!"float4*", !"image2d_t", !"sampler_t", !"float2", !"float2", !"float2"}
!7 = !{!"float __attribute__((ext_vector_type(4)))*", !"image2d_t", !"sampler_t", !"float __attribute__((ext_vector_type(2)))", !"float __attribute__((ext_vector_type(2)))", !"float __attribute__((ext_vector_type(2)))"}
!8 = !{!"", !"", !"", !"", !"", !""}

Note that the declaration of my_lib_func is not annotated but it's also can be implementing (I think it will not require much work).

What is your opinion if the function args is the only place where we strongly need pointer annotation at the moment?

[bader]

What is your opinion if the function args is the only place where we strongly need pointer annotation at the moment?

I'm not sure if annotating function parameters is enough to recover SPIR-V types correctly in all cases, but I hope so. The example I provided came to my mind after reading how types are suggested to be recovered at the moment.

I have a few questions regarding this proposal:

• How difficult would it be to infer particular type within LLVM module with having function parameter annotation? I'd like to avoid complex algorithms requiring instructions traversal.
• How reliable are function metadata (i.e. can we be sure that LLVM passes preserve these metadata)?
• The OpenCL kernel argument types metadata are optional (i.e. can be omitted) and applied to OpenCL kernels only. Do I understand it correctly that the proposal is to add new metadata (similar to OpenCL kernel argument types), which will be required for SPIR target and must be emitted for non-OpenCL languages as well (e.g. SYCL/HIP/OpenMP/CUDA/etc.)?

[jcranmer-intel]

Some thoughts from my experiences trying to implement the type scavenger:

Firstly, a bidirectional approach is rather more difficult to implement. It is really better to be able to have the pointee type known at the value source, and then push through the code and insert bitcasts when the use implies a pointee type different from the source type.

With this in mind, I can generally categorize pointer-typed values into three categories. They are as follows (keep in mind that the rules for constant expressions are the same as their corresponding instructions):

1. The pointee type is given by the value itself. This is true for alloca, getelementptr, GlobalVariable, Function, and blockaddress.
2. The value propagates pointee type from its operands to its output. This is true for addrspacecast (well, technically it can bitcast, but we don't want it to do that even in typed LLVM today), phi, select, and freeze.
3. The value itself gives no clue as to its pointee type. This is true for CallBase (which can return a pointer-typed value), extractelement (but SPIR-V makes vector-of-pointers illegal), extractvalue, load (if you're loading a pointer from memory), bitcast (which shouldn't exist in opaque pointer mode anyways), va_arg (illegal in SPIR-V I think), inttoptr, undef, poison, null.

There's one thing I haven't included in this list, and that's Argument... which can live in any of the categories depending on circumstances. If it has an sret or similar parameter, then it lives in the first category. If it's an internal function call (where both callee and caller are defined in the module), then it's in the second category; otherwise, it's in the third. (Also note that function return types have similar concerns to arguments).

I will also note that, where LLVM tells us the pointee type of the value, we get at best a single level of pointer indirection. If an alloca stores a ptr, we don't know if that's an i8* or a %struct "opencl.block"* pointer without doing inference. In part do to this reasoning, I've been trying to make everything work with only scavenging a single level of indirection, which makes the lowering for anything doing memory operations somewhat more complicated than I've alluded to here in prior comments. I'm not entirely happy with how this works out, but I'm also not immediately seeing any solution that's better.

From the perspective of my implementation attempts, having known-correct types at the function boundary makes implementation simpler. a single reverse-postorder traversal of every function would type it sufficiently well, and even reverse-postorder may not be necessary. In essence, if the third category doesn't exist (or you can at least get something working by treating ptr as i8* and inserting bitcasts as necessary), then the scavenging problem is less a scavenging problem and more a where-to-insert-bitcast problem. There's an alternative implementation idea I've thought about, but haven't tested, that instead revolves around making values of the second class get a deferred type such that all will be forced to use the same type when we see the first typed use of any of those values. (Function pointers are an especially difficult problem to think about that I'm completely punting on for the moment).

A rather more important issue, though, is the need to be able to positively identify what pointee type a given use of a pointer expects it to be. Modulo pointers-to-pointers issues, the only instructions that don't immediately have identifiable pointee type uses are bitcast and ptr2int (both of which are likely usable only where bitcast-to-i8* is a feasible solution in the first place), insertvalue (which I don't think I've ever seen in SPIR-V code), and call instructions. especially to unknown extern functions. It's annoying when it comes to something like __spirv_AtomicLoad, because the type needs to be known for correctness and unlike the existing uses of the demangler in #1477, I need to support more than just the special opaque types. That's a use case where I'd personally prefer just slapping elementtype attributes on everything, but those work only on LLVM intrinsics, which these function calls are not.

Answering the questions:

* How difficult would it be to infer particular type within LLVM module with having function parameter annotation? I'd like to avoid complex algorithms requiring instructions traversal.

Instruction traversal is required anyways, to insert bitcasts where needed. To me, the question isn't can you avoid instruction traversal, but rather, can you avoid visiting each instruction more than once.

* How reliable are function metadata (i.e. can we be sure that LLVM passes preserve these metadata)?

The design of metadata is supposed to be that it's correct to drop all metadata, and passes should drop all metadata they don't understand. The latter requirement tends to be frequently flouted by optimization passes, but metadata on functions is even less likely to be disturbed since you're never considering that you might be changing the semantics of a function.

* The OpenCL kernel argument types metadata are optional (i.e. can be omitted) and applied to OpenCL kernels only. Do I understand it correctly that the proposal is to add new metadata (similar to OpenCL kernel argument types), which will be required for SPIR target and must be emitted for non-OpenCL languages as well (e.g. SYCL/HIP/OpenMP/CUDA/etc.)?

At some point, I believe, there is a need for frontends to change their IR to cope with LLVM's opaque pointers. Adding function metadata is one of way doing it, but it might not be the best way; another idea I'd suggest (assuming that the SPIR-V opaque types are the main issue here) is to make each type a pointer in a different address space.

[svenvh]

Thanks for sharing your experiences @jcranmer-intel. Just wanted to highlight one point you're touching on:

another idea I'd suggest (assuming that the SPIR-V opaque types are the main issue here) is to make each type a pointer in a different address space.

I've been thinking about this too, but haven't had the time to play with this yet. It does make sense conceptually, since one opaque type generally can't be casted to another, and I think this could be a more robust solution than metadata.

[jcranmer-intel]

A status update:

I've uploaded #1499, which hits a very important milestone: with that PR, it is possible to compile the most basic of basic kernels in opaque pointer mode (I flipped the simplest test I could find to use opaque pointers to prove so).

I'm not yet certain enough of the type scavenger's structure to fold that into a patch for review just yet, but the current pass rate I have with my local version when I compiled the SYCL test suite with opaque pointers is 90%. I don't have any numbers for OpenCL test suite pass rates, because those tend to filter through mangleBuiltin, whose use of getPointerElementType I haven't been able to fix yet. This is without any frontend changes, though I'll admit that many of the failures I see appear to be related to the special SPIR-V opaque types in particular.

Something I'll try in the near future is to make the test suite check SPIR-V-to-LLVM conversion using opaque pointers, which should work already.

[iliya-diyachkov]

By the way I have submitted the draft patch which I mentioned before (it hints arguments of non-kernel functions) so you can build and try patched clang.

[jcranmer-intel]

Another status update: I have just uploaded #1512, which implements type scavenging well enough to mass-revert the -opaque-pointers=0 flag in LLVM IR tests, although I consider it inadequate (particularly in the testing department, I haven't had the time yet to write more thorough tests for nested pointers or other complex scenarios I've seen in other test suites). But it lets people generate SPIR-V for LLVM opaque pointers in a significant number of cases!

[jcranmer-intel]

If you're not aware yet, I've posted on RFC on discourse about opaque pointers and SPIR-V here: https://discourse.llvm.org/t/rfc-better-support-for-typed-pointers-in-an-opaque-pointer-world/63339

[jcranmer-intel]

Status update time again: with the landing of #1512, we are down to two calls to getPointerElementType: the regularize fix for the JointMatrixINTEL type (which I haven't started investigating), and the use in transTypeDesc, which is really from mangleBuiltin. In addition to these calls, there are additional issues of incomplete scavenging of pointer types as well as areas where LLVM optimizations prior to llvm-spirv being invoked screwing code up.

My transition plan for mangleBuiltin is as follows. The builtin mangling struct details, which already contains instructions for encoding signedness or constness and the like, will be extended to include pointer element type information, and this communicates the information to transTypeDesc itself--pushing the getPointerElementType call up the call stack into mangleBuiltin. This is pushed further up to the callers of mangleBuiltin by having those callers identify a ArrayRef<Type *> pointer element type array, which mirrors the information that is extracted from getParameterTypes. Building this out completely is taking some time, as this means that mutateCallInst needs to change to be able to let the mutation function also indicate changes to the parameter types, which is more call sites than anything else I've inspected to date.

The biggest wrinkle in this plan is that a few of the builtins take an array of opaque types, which are currently represented as something like %__spirv_DeviceEvent**. Since only a few builtins are impacted, it is possible to work around this for now, but I would like to see the IR representations of these opaque types change, to the new opaque type representation I've suggested in the RFC. However, this still requires changes to LLVM itself, and I am not confident that such changes will be able to make it in before the LLVM 15 release, so I'm currently trying to prioritize getting as much working with the current representation as possible.

mangleBuiltin not working with opaque pointers is currently the biggest blocker for getting opaque pointers enabled, I believe. With my local patches, turning the reader mode to enabling opaque pointers causes about a quarter of the tests to fail, and spot-checking the results suggests that about half of the failures are do to check lines expecting typed pointer IR and the other half are do to mutateCallInst or addCallInst calls that I haven't ported yet.

[jcranmer-intel]

Status update, since I've realized I haven't done this for a while:

intel/llvm#6535 is the frontend change that removes the need for the last remaining call to getPointerElementType for the JointMatrixINTEL type renaming. #1570 is the change that removes the actual call in this repository (or, rather, moves it to getNonOpaquePointerElementType so that the legacy IR may be handled until opaque pointer transition is complete).

The final major piece for opaque pointer enabling is fixing the calls to mutateCallInst to pass in pointer types to mangleBuiltin correctly. I'm planning to tackle this via the mutator-based approach proposed in #1541. There's a couple of stages to doing this, though: first I need to land the improved demangler support (I should have that patch up shortly), and then the mutator infrastructure, and finally, the actual changes from mutateCallInst to the mutator.

In parallel to that effort, what still remains to be done is porting tests to use opaque pointers instead of typed pointers, as well as fixing regressions that happen when you turn on opaque pointers--there are likely to be quite a few of them! Based on some of my previous test runs in opaque pointer mode, we are likely to be better served by introducing an LLVM opaque type (as proposed in the link at #1444 (comment)), but I haven't worked out a transition plan for that yet.

To give that famous quote, this is not the end. It is not even the beginning of the end. But it is, perhaps, the end of the beginning.

[aeubanks]

Any update on this?

[jcranmer-intel]

As of a few weeks ago (sorry for not updating this issue sooner), the code no longer calls getPointerElementType on non-opaque pointers. Many kernels will work when compiled with opaque pointers--I get about a 90% pass rate in the test suites I'm using to gauge progress--but that is still a far cry from "all".

Right now, my focus is on getting opaque types added into LLVM (https://reviews.llvm.org/D135202), which is necessary to fix some of the issues with using the special SPIR-V image and sampler types. Beyond that, there are still a handful of lit tests that fail to work with clang-cl -opaque-pointers that need addressing, and there's a few things that are likely to be generally broken (predominantly the function pointer extension stuff).

[svenvh]

LLVM have clarified the timeline for typed pointer support: https://reviews.llvm.org/D140487

Once release/16.x has been branched, typed pointers on main will no longer be supported (and can be actively broken).

[aeubanks]

typed pointer support is going away in a week, any update now that https://reviews.llvm.org/D135202 landed?

[MrSidims]

@aeubanks there is also an important clang PR https://reviews.llvm.org/D141008 that should be merged for OpenCL types support. And it has the appropriate counterpart here in the translator #1799

[jcranmer-intel]

Once the things that @MrSidims refers to have landed, I will consider opaque pointer support conceptually complete, but there are still some bugs in opaque pointer support that haven't been fully fixed.

[karolherbst]

fyi, I've opened this PR recently which adds OpBitcast instruction for mismatching pointer argument types if allowed by the callee: KhronosGroup/SPIRV-Tools#5534

So with this, spirvs can be linked regardless of the specific pointer types in imported function signatures.

[alan-baker]

FYI, SPV_KHR_untyped_pointers was provisionally released which would allow a more direct translation if supported by the implementation.

[MrSidims]

AFAIK @vmaksimo is working on its implementation

[MrSidims]

Closing this issue, will open one for SPV_KHR_untyped_pointers separately.