[InferAddressSpaces] Handle unconverted ptrmask #140802
Conversation
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-backend-amdgpu Author: Robert Imschweiler (ro-i) ChangesIn case a ptrmask cannot be converted to the new address space due to an unknown mask value, this needs to be detcted and an addrspacecast is needed to not hinder a future use of the unconverted return value of ptrmask. Otherwise, users of this value will become invalid by receiving a nullptr as an operand. This LLVM defect was identified via the AMD Fuzzing project. (See https://reviews.llvm.org/D80129 for an explanation of why some ptrmasks are impossible to convert to other addrspaces.) Full diff: https://github.com/llvm/llvm-project/pull/140802.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp b/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
index d3771c0903456..4f2e8bbd1102a 100644
--- a/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
+++ b/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
@@ -1338,7 +1338,20 @@ bool InferAddressSpacesImpl::rewriteWithNewAddressSpaces(
unsigned OperandNo = PoisonUse->getOperandNo();
assert(isa<PoisonValue>(NewV->getOperand(OperandNo)));
- NewV->setOperand(OperandNo, ValueWithNewAddrSpace.lookup(PoisonUse->get()));
+ WeakTrackingVH NewOp = ValueWithNewAddrSpace.lookup(PoisonUse->get());
+ if (NewOp) {
+ NewV->setOperand(OperandNo, NewOp);
+ } else {
+ // Something went wrong while converting the instruction defining the new
+ // operand value. -> Replace the poison value with the previous operand
+ // value combined with an addrspace case.
+ Value *PoisonOp = NewV->getOperand(OperandNo);
+ Value *OldOp = V->getOperand(OperandNo);
+ Value *AddrSpaceCast =
+ new AddrSpaceCastInst(OldOp, PoisonOp->getType(), "",
+ cast<Instruction>(NewV)->getIterator());
+ NewV->setOperand(OperandNo, AddrSpaceCast);
+ }
}
SmallVector<Instruction *, 16> DeadInstructions;
diff --git a/llvm/test/Transforms/InferAddressSpaces/AMDGPU/ptrmask.ll b/llvm/test/Transforms/InferAddressSpaces/AMDGPU/ptrmask.ll
index 6ef926f935830..1c1d1df79520d 100644
--- a/llvm/test/Transforms/InferAddressSpaces/AMDGPU/ptrmask.ll
+++ b/llvm/test/Transforms/InferAddressSpaces/AMDGPU/ptrmask.ll
@@ -343,6 +343,24 @@ define i8 @ptrmask_cast_local_to_flat_load_range_mask(ptr addrspace(3) %src.ptr,
ret i8 %load
}
+; Non-const masks with no known range should not prevent other ptr-manipulating
+; instructions (such as gep) from being converted.
+define i8 @ptrmask_cast_local_to_flat_unknown_mask(ptr addrspace(3) %src.ptr, i64 %mask, i64 %idx) {
+; CHECK-LABEL: @ptrmask_cast_local_to_flat_unknown_mask(
+; CHECK-NEXT: [[CAST:%.*]] = addrspacecast ptr addrspace(3) [[SRC_PTR:%.*]] to ptr
+; CHECK-NEXT: [[MASKED:%.*]] = call ptr @llvm.ptrmask.p0.i64(ptr [[CAST]], i64 [[MASK:%.*]])
+; CHECK-NEXT: [[TMP1:%.*]] = addrspacecast ptr [[MASKED]] to ptr addrspace(3)
+; CHECK-NEXT: [[GEP:%.*]] = getelementptr i8, ptr addrspace(3) [[TMP1]], i64 [[IDX:%.*]]
+; CHECK-NEXT: [[LOAD:%.*]] = load i8, ptr addrspace(3) [[GEP]], align 1
+; CHECK-NEXT: ret i8 [[LOAD]]
+;
+ %cast = addrspacecast ptr addrspace(3) %src.ptr to ptr
+ %masked = call ptr @llvm.ptrmask.p0.i64(ptr %cast, i64 %mask)
+ %gep = getelementptr i8, ptr %masked, i64 %idx
+ %load = load i8, ptr %gep
+ ret i8 %load
+}
+
declare ptr @llvm.ptrmask.p0.i64(ptr, i64) #0
declare ptr addrspace(5) @llvm.ptrmask.p5.i32(ptr addrspace(5), i32) #0
declare ptr addrspace(3) @llvm.ptrmask.p3.i32(ptr addrspace(3), i32) #0
|
|
(note: will review properly after the holiday) |
In case a ptrmask cannot be converted to the new address space due to an unknown mask value, this needs to be detcted and an addrspacecast is needed to not hinder a future use of the unconverted return value of ptrmask. Otherwise, users of this value will become invalid by receiving a nullptr as an operand. This LLVM defect was identified via the AMD Fuzzing project.
9102778 to
af027cb
Compare
|
Rebased and resolved conflict |
|
Ping. |
| // (e.g. 32-bit) casts work by chopping off the high bits. | ||
| if (FromASBitSize < ToASBitSize) | ||
| return 0; | ||
| return ToASBitSize; |
There was a problem hiding this comment.
These implicit conversions from unsigned are broken. I think the KnownBits constructors should have explicit added.
This should always return a KnownBits with the correct bitwidth and change the mask value, not the bitwidth
There was a problem hiding this comment.
I think the KnownBits constructors should have explicit added.
do you dislike the implicit constructor of KnownBits because it's intuitively a bit ambiguous if the unsigned argument stands for "number of known bits" or "bitwidth"?
| computeKnownBitsAddrSpaceCast(unsigned FromAS, unsigned ToAS, | ||
| const Value &PtrOp) const; | ||
|
|
||
| LLVM_ABI KnownBits computeKnownBitsAddrSpaceCast( |
There was a problem hiding this comment.
Why do these two functions have to be in TTI?
There was a problem hiding this comment.
Isn't that the right place? Targets can override the semantic that way if they like. And since the function is split into two parts, it's easier to override only the part you need to.
There was a problem hiding this comment.
I mean, yes and no. The no part is that I don't see any target-dependent part here that can't be handled with information from the data layout.
There was a problem hiding this comment.
The addrspacecast. For example, the case where I cast larger address to smaller address, I just trunc. This might be valid for all practical cases (given integral), but it's in no way guaranteed afaiu.
Also, the anyext for smaller->larger casts is a conservative choice that could be improved with target knowledge, ig
There was a problem hiding this comment.
The addrspacecast.
Then I'd just leave the cast between two address spaces part into TTI, which makes sense, and then leave no default implementation. Even for us, with GAS, AS5->AS0 is not zero extension.
Co-authored-by: Shilei Tian <i@tianshilei.me>
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
In case a ptrmask cannot be converted to the new address space due to an unknown mask value, this needs to be detcted and an addrspacecast is needed to not hinder a future use of the unconverted return value of ptrmask. Otherwise, users of this value will become invalid by receiving a nullptr as an operand.
This LLVM defect was identified via the AMD Fuzzing project.
(See https://reviews.llvm.org/D80129 for an explanation of why some ptrmasks are impossible to convert to other addrspaces.)