[SYCL] Addrspace cast phi operands when lowering conditional operator #768
Conversation
clang/lib/CodeGen/CGExpr.cpp
Outdated
| phi->addIncoming(rhs->getPointer(), rhsBlock); | ||
| llvm::Value *lhsPtr = lhs->getPointer(); | ||
| llvm::Value *rhsPtr = rhs->getPointer(); | ||
| if (rhsPtr->getType() != lhsPtr->getType()) |
There was a problem hiding this comment.
JIC, is it guaranteed that rhsPtr/lhsPtr are non-nullptr?
There was a problem hiding this comment.
I think it is. In any case I am not changing behavior with regards to this part. So if it was or was not guaranteed earlier the same behavior remains now.
clang/lib/CodeGen/CGExpr.cpp
Outdated
| llvm::Value *lhsPtr = lhs->getPointer(); | ||
| llvm::Value *rhsPtr = rhs->getPointer(); | ||
| if (rhsPtr->getType() != lhsPtr->getType()) | ||
| rhsPtr = Builder.CreatePointerBitCastOrAddrSpaceCast(rhsPtr, |
There was a problem hiding this comment.
This is not always legal. If RHS is a local pointer and LHS is a global pointer, the cast is "local->global", which is invalid.
If pointer address spaces are different, the only correct cast is a cast to generic (addrspace 4). Check the #388, where a similar change was made around select instruction.
There was a problem hiding this comment.
Added casts to generic address space as needed.
sndmitriev
force-pushed
the
public/sndmitriev/addrspace-cond-op
branch
from
0bb9032 to
c34fbc6
Compare
clang/lib/CodeGen/CGExpr.cpp
Outdated
| llvm::Value *rhsPtr = rhs->getPointer(); | ||
| if (rhsPtr->getType() != lhsPtr->getType()) { | ||
| if (getLangOpts().SYCLIsDevice) { | ||
| auto CastToAS = [](llvm::Value *V, llvm::BasicBlock *BB, unsigned AS) { |
There was a problem hiding this comment.
insertAddressSpaceCast from #388 seems to do exactly the same, and IIRC, this will be a third place where we need it. Can you use it here as well?
There was a problem hiding this comment.
insertAddressSpaceCast from #388 uses some assumptions which I believe are not legal here. For example on line 4155 the input value V is casted to Instruction. I do not think we can guarantee that lhsPtr or rhsPtr here is an Instruction.
There was a problem hiding this comment.
I see. Well, you can refactor that function to take an insertion point, but this is a minor problem, so feel free to ignore it if you don't think that it worth the effort.
Make sure that all operands of phi instruction that is generated while lowering conditional operator have the same type. This fixes compiler assertion. Signed-off-by: Sergey Dmitriev <[email protected]>
sndmitriev
force-pushed
the
public/sndmitriev/addrspace-cond-op
branch
from
c34fbc6 to
c9d6415
Compare
|
If there are no more comments can someone please approve this PR? |
* Add ReadNone attr for Builtin functions Signed-off-by: Aleksander Fadeev <[email protected]>
Make sure that all operands of phi instruction that is generated while
lowering conditional operator have the same type. This fixes compiler
assertion.
Signed-off-by: Sergey Dmitriev [email protected]