Fix #5135: Prevent CondExp from dropping address spaces#5144
Fix #5135: Prevent CondExp from dropping address spaces#5144anushkagupta200615-jpg wants to merge 10 commits into
Conversation
This fixes an issue in the DCompute backend where ternary condition expressions dropped device address spaces. R-values now use local allocas for the value, and L-values natively preserve pointer types through LLVM PHI nodes.
|
Needs a test case.
You could use this :) |
gulugulubing
left a comment
gulugulubing
left a comment
There was a problem hiding this comment.
Thanks for working on this fix!
| if (retPtr && u->type->toBasetype()->ty != TY::Tnoreturn) { | ||
| LLValue *lval = makeLValue(e->loc, u); | ||
| DtoStore(lval, retPtr); | ||
| if (u->type->toBasetype()->ty != TY::Tnoreturn) { |
There was a problem hiding this comment.
if (u && u->type->toBasetype()->ty != TY::Tnoreturn) { may fix the ci.
| if (retPtr && v->type->toBasetype()->ty != TY::Tnoreturn) { | ||
| LLValue *lval = makeLValue(e->loc, v); | ||
| DtoStore(lval, retPtr); | ||
| if (v->type->toBasetype()->ty != TY::Tnoreturn) { |
There was a problem hiding this comment.
Similarly, if (v && v->type->toBasetype()->ty != TY::Tnoreturn)
|
@anushkagupta200615-jpg Add if |
| phi->addIncoming(u_val ? u_val : llvm::UndefValue::get(ptrType), u_bb); | ||
| LLValue *v_inc = v_val ? v_val : llvm::UndefValue::get(ptrType); | ||
| if (v_inc->getType() != ptrType) { |
There was a problem hiding this comment.
With opaque pointers these types only differ by address space, and bitcast across address spaces is invalid (needs addrspacecast). Since preserving the address space is the whole point of this PR, unifying via bitcast looks like it would either assert or drop the space again . what's the intended behavior when the two branches have different address spaces?
Replace DtoRVal + DtoStoreZextI8 with DtoAssign, which correctly handles types that cannot be loaded into a register (e.g. large structs). The previous approach triggered 'getRVal() for memory-only type' assertion in DLValue::getRVal().
|
The CI failures are caused by DtoRVal() crashing on memory-only types in the rvalue path. I've submitted a fix PR to your branch: anushkagupta200615-jpg#1. Merging it should solve this problem. |
Fix memory-only type crash in rvalue CondExp path
|
Hi @anushkagupta200615-jpg, I've been looking into the CI failures(I reported #5135 originally). The rvalue path was straightforward to fix, but the remaining lvalue failure (ternary_lval_assign test) is more subtle than expected. |
|
@gulugulubing the failing test is unsupported for LLVM18 , couldnt reproduce it:) took some effort to figure out the exact problem .. Also i am not very much sure about the side effects(may or may not be) @JohanEngelen @thewilsonator please have a look. |
4 participants
This fixes an issue in the DCompute backend where ternary condition expressions dropped device address spaces. R-values now use local allocas for the value, and L-values natively preserve pointer types through LLVM PHI nodes.
Fixes #5135.
This PR fixes a bug in the DCompute backend where ternary condition expressions
(c) ? e1 : e2stripped device address spaces (e.g.,addrspace(1)) fromGlobalPointermemory references.Root Cause:
Previously,
visit(CondExp)indiscriminately wrapped the branches inside a generic address space temporary (condtmppointer), which forcedDSpecialRefValue::getRVal()to reload the branches usinggetOpaquePtrType(). This stripped out the inner device address space.Solution:
The code has been updated to distinctly handle ternary expressions depending on their evaluation context:
e->isLvalue()): Entirely bypasses intermediate allocation wrappers andDSpecialRefValue. Instead, it fetches the actual device pointers usingmakeLValueand naturally preserves their exact LLVM type (includingaddrspace) by linking them into an LLVMPHINodeat the branch merge point.!e->isLvalue()): Creates a standardallocafor the target value type (rather than anallocaof a pointer), evaluates the condition branches as standard R-values, and stores them appropriately. This prevents any inner structs (likeGlobalPointer) from having their address spaces stripped from memory.This fully resolves
val = (c) ? input[i] : 0as well as the relatedval = (c) ? input[i] : input[j]bug.