Implemented conversion of llvm.bswap intrinsic to SPIR-V, closes #210 #221
Conversation
| //This is mostly the same logic as lib/CodeGen/IntrinsicLowering.cpp#LowerBSWAP | ||
| default: llvm_unreachable("Unhandled type size of value to byteswap!"); | ||
| case 16: { | ||
| SPIRVValue *Offset8 = BM->addConstant(ArgType, 8); |
There was a problem hiding this comment.
This function does not guarantee the uniqueness of the constant, e.g., you can create two SPIR-V constants with different id's but the same value if you called it twice. Therefore it is users' responsibility to maintain the uniqueness of the constants. When it is called during LLVM/SPIR-V translation it is fine since a map is used to maintain the uniqueness. However it is not suitable to be used to create SPIR-V on the fly.
Ideally, it is desired to improve addConstant so that uniqueness is maintained, e.g. by using the LLVM FoldingSet.
A temporary workaround may be to create the LLVM constant first, then translate it to SPIR-V.
There was a problem hiding this comment.
I would implement the uniqueness of constants via a map (uint64_t to SPIRVValue*) or a set of SPIRVValue* values, which can be checked by addConstant whether a constant already exists and then either inserting a new one or returning the existing.
Is there any reason, you would prefer a LLVM FoldingSet?
There was a problem hiding this comment.
LLVM FoldingSet is a more generic solution. By implementing profile of a SPIRV entity, we can make any SPIRV entity unique, e.g. SpecConstantOp. Similar approach can be extended to SPIRV types and attributes. Essentially this can open the door for a generic solution of uniqueness for other SPIRV entities.
In the beginning, the SPIRV in-memory representation was designed to be independent of LLVM, in the hope of being useful to projects that do not want to depend on LLVM. However, it seems this restriction is not so useful and unnecessarily limits the utility available. Therefore I would like to allow SPIRV in-memory representation to use LLVM utilities.
No description provided.