Improve matrices translation with SPV_KHR_untyped_pointers #2857
Conversation
vmaksimo
changed the title
| ; CHECK-SPIRV: UntypedAccessChainKHR [[#TypePtr]] [[#Res:]] [[#TypeMatrix]] [[#VarMatrixPtr]] [[#Const1]] | ||
| ; CHECK-SPIRV: Store [[#Res]] [[#Const42]] | ||
|
|
||
| ; CHECK-LLVM: %0 = alloca target("spirv.CooperativeMatrixKHR", i32, 3, 12, 12, 0) |
There was a problem hiding this comment.
| ; CHECK-LLVM: %0 = alloca target("spirv.CooperativeMatrixKHR", i32, 3, 12, 12, 0) | |
| ; CHECK-LLVM: %[[#Matrix:]] = alloca target("spirv.CooperativeMatrixKHR", i32, 3, 12, 12, 0) |
and so on
There was a problem hiding this comment.
It's basically just a copy of access_store.ll test :) do we need these checks there too?
There was a problem hiding this comment.
May be in another PR, not here :)
| else if (RetTy->isVectorTy()) | ||
| Ty = cast<VectorType>(RetTy)->getElementType(); | ||
| else | ||
| Ty = RetTy; |
There was a problem hiding this comment.
So for store pointer parameter would be considered as void *? If so it doesn't feel right, can we instead query type of the value operand of the store?
There was a problem hiding this comment.
Sorry, I don't get this at all. Don't we already query the type from the llvm instructions? Or do you mean specific SPIR-V opcode in mind?
There was a problem hiding this comment.
On the line 3437 RetTy is defined like this:
Type *RetTy =
BI->hasType() ? transType(BI->getType()) : Type::getVoidTy(*Context);
So if the instruction doesn't have a return type (aka ...Store...) RetTy will be void and the line 3473 && RetTy is always true.
When processing Ptr argument of ...Store... instruction isa<Argument>(Val) will also be true, if so, we assign Ty to RetTy which is void.
Is it correct or am I missing something?
There was a problem hiding this comment.
Got you, you're right - it won't be correct, i was just "lucky" that we didn't have test case for that. Will prepare a fix
| auto *AI = static_cast<SPIRVAtomicInstBase *>(BI); | ||
| ArgTys[Ptr] = TypedPointerType::get( | ||
| transType(AI->getSemanticType()), | ||
| SPIRSPIRVAddrSpaceMap::rmap( | ||
| BI->getValueType(Ops[Ptr]->getId())->getPointerStorageClass())); | ||
| } else if (OC == spv::OpCooperativeMatrixStoreKHR || | ||
| OC == spv::internal::OpJointMatrixStoreINTEL || |
There was a problem hiding this comment.
BTW, we can stop modifying code related to JointMatrixINTEL instructions that have CooperativeMatrixKHR counterparts aka trivial MatrixLoad/Store, MulAdd etc as they will be depricated. Meanwhile OpCooperativeMatrixStoreCheckedINTEL and other unique for SPV_INTEL_joint_matrix extension instruction must still be handled properly.
| else if (RetTy->isVectorTy()) | ||
| Ty = cast<VectorType>(RetTy)->getElementType(); | ||
| else | ||
| Ty = RetTy; |
There was a problem hiding this comment.
On the line 3437 RetTy is defined like this:
Type *RetTy =
BI->hasType() ? transType(BI->getType()) : Type::getVoidTy(*Context);
So if the instruction doesn't have a return type (aka ...Store...) RetTy will be void and the line 3473 && RetTy is always true.
When processing Ptr argument of ...Store... instruction isa<Argument>(Val) will also be true, if so, we assign Ty to RetTy which is void.
Is it correct or am I missing something?
Ensure that we do correct translation of matrices (
SPV_KHR_cooperative_matrixandSPV_INTEL_joint_matrixextensions) when untyped pointers are enabled.This mainly fixes mangling issues in reverse translation for the untyped pointer.
Also added handling for typed and untyped SPIR-V friendly access chain instructions in forward translation (a point to review and discuss).