[SelectionDAGBuilder] Use address width when lowering ptrtoaddr #139423
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.6-beta.1
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-llvm-selectiondag Author: Alexander Richardson (arichardson) ChangesInstead of just deferring to ptrtoint, we should truncate to the index Full diff: https://github.com/llvm/llvm-project/pull/139423.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index e6651d000bd71..806bab5379bde 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -3878,7 +3878,20 @@ void SelectionDAGBuilder::visitSIToFP(const User &I) {
}
void SelectionDAGBuilder::visitPtrToAddr(const User &I) {
- visitPtrToInt(I);
+ const auto &TLI = DAG.getTargetLoweringInfo();
+ const DataLayout &DL = DAG.getDataLayout();
+ LLVMContext &Ctx = *DAG.getContext();
+ // ptrtoaddr is equivalent to a truncate of ptrtoint to address/index width
+ SDValue N = getValue(I.getOperand(0));
+ Type *PtrTy = I.getOperand(0)->getType();
+ EVT AddrVT = EVT::getIntegerVT(Ctx, DL.getPointerAddressSizeInBits(PtrTy));
+ if (auto *VTy = dyn_cast<VectorType>(PtrTy)) {
+ Type *EltTy = VTy->getElementType();
+ AddrVT = EVT::getVectorVT(Ctx, AddrVT, VTy->getElementCount());
+ }
+ N = DAG.getPtrExtOrTrunc(N, getCurSDLoc(), AddrVT);
+ N = DAG.getZExtOrTrunc(N, getCurSDLoc(), TLI.getValueType(DL, I.getType()));
+ setValue(&I, N);
}
void SelectionDAGBuilder::visitPtrToInt(const User &I) {
diff --git a/llvm/test/CodeGen/AMDGPU/ptrtoint-ptrtoaddr-p8.ll b/llvm/test/CodeGen/AMDGPU/ptrtoint-ptrtoaddr-p8.ll
index 32b5d9441b61c..da4b531ab5b25 100644
--- a/llvm/test/CodeGen/AMDGPU/ptrtoint-ptrtoaddr-p8.ll
+++ b/llvm/test/CodeGen/AMDGPU/ptrtoint-ptrtoaddr-p8.ll
@@ -32,8 +32,9 @@ define <2 x i64> @ptrtoaddr_vec(<2 x ptr addrspace(8)> %ptr) {
; CHECK-LABEL: ptrtoaddr_vec:
; CHECK: ; %bb.0:
; CHECK-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; CHECK-NEXT: v_mov_b32_e32 v3, v5
; CHECK-NEXT: v_mov_b32_e32 v2, v4
+; CHECK-NEXT: v_and_b32_e32 v1, 0xffff, v1
+; CHECK-NEXT: v_and_b32_e32 v3, 0xffff, v5
; CHECK-NEXT: s_setpc_b64 s[30:31]
%ret = ptrtoaddr <2 x ptr addrspace(8)> %ptr to <2 x i64>
ret <2 x i64> %ret
@@ -57,6 +58,9 @@ define i128 @ptrtoaddr_ext(ptr addrspace(8) %ptr) {
; CHECK-LABEL: ptrtoaddr_ext:
; CHECK: ; %bb.0:
; CHECK-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; CHECK-NEXT: v_and_b32_e32 v1, 0xffff, v1
+; CHECK-NEXT: v_mov_b32_e32 v2, 0
+; CHECK-NEXT: v_mov_b32_e32 v3, 0
; CHECK-NEXT: s_setpc_b64 s[30:31]
%ret = ptrtoaddr ptr addrspace(8) %ptr to i128
ret i128 %ret
|
Created using spr 1.3.6-beta.1
arsenm
approved these changes
May 21, 2025
arsenm
reviewed
May 21, 2025
| EVT AddrVT = EVT::getIntegerVT(Ctx, DL.getPointerAddressSizeInBits(PtrTy)); | ||
| if (auto *VTy = dyn_cast<VectorType>(PtrTy)) | ||
| AddrVT = EVT::getVectorVT(Ctx, AddrVT, VTy->getElementCount()); | ||
| N = DAG.getPtrExtOrTrunc(N, getCurSDLoc(), AddrVT); |
There was a problem hiding this comment.
The LangRef says that it's always zero extend, but DAG.getPtrExtOrTrunc's comment suggests it may decide to be another extension some day?
Created using spr 1.3.6-beta.1
|
Since we now always have to truncate to address width there are no more test changes and this becomes effectively NFC. |
nikic
approved these changes
Aug 11, 2025
arsenm
reviewed
Aug 12, 2025
| const auto &TLI = DAG.getTargetLoweringInfo(); | ||
| const DataLayout &DL = DAG.getDataLayout(); | ||
| // ptrtoaddr is equivalent to a truncate of ptrtoint to address/index width | ||
| auto Op0 = I.getOperand(0); |
There was a problem hiding this comment.
Suggested change
| auto Op0 = I.getOperand(0); | |
| const Value *Op0 = I.getOperand(0); |
No auto
| auto Op0 = I.getOperand(0); | ||
| SDValue N = getValue(Op0); | ||
| // By definition the type of the ptrtoaddr must be equal to the address type. | ||
| assert(I.getType() == DL.getAddressType(Op0->getType())); |
There was a problem hiding this comment.
Don't need to assert things enforced by the verifier
| // By definition the type of the ptrtoaddr must be equal to the address type. | ||
| assert(I.getType() == DL.getAddressType(Op0->getType())); | ||
| EVT AddrVT = TLI.getValueType(DL, I.getType()); | ||
| N = DAG.getNode(ISD::TRUNCATE, getCurSDLoc(), AddrVT, N); |
There was a problem hiding this comment.
If these are the same type, why do you need the truncate?
There was a problem hiding this comment.
While most targets will have them be the same, the Op0 type can be wider. This is the case e.g. for the AMD GPU buffer pointers.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of just deferring to ptrtoint, we should truncate to the index
width and then perform the ZextOrTrunc.
This is effectively NFC since ptrtoint ends up doing the same thing, but
handling it explicitly is cleaner and will make it easier to eventually
upstream the changes needed for CHERI support.