[mlir][LLVMIR] Add folders for llvm.inttoptr and llvm.ptrtoint
#141891
Conversation
This PR adds folders for `inttoptr(ptrtoint(x))` and `ptrtoint(inttoptr(x))`.
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-llvm Author: Diego Caballero (dcaballe) ChangesThis PR adds folders for Full diff: https://github.com/llvm/llvm-project/pull/141891.diff 3 Files Affected:
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
index 6fde45ce5c556..3eb48964bd378 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
@@ -622,10 +622,14 @@ def LLVM_AddrSpaceCastOp : LLVM_CastOp<"addrspacecast", "AddrSpaceCast",
}
def LLVM_IntToPtrOp : LLVM_DereferenceableCastOp<"inttoptr", "IntToPtr",
LLVM_ScalarOrVectorOf<AnySignlessInteger>,
- LLVM_ScalarOrVectorOf<LLVM_AnyPointer>>;
+ LLVM_ScalarOrVectorOf<LLVM_AnyPointer>> {
+ let hasFolder = 1;
+}
def LLVM_PtrToIntOp : LLVM_CastOp<"ptrtoint", "PtrToInt",
LLVM_ScalarOrVectorOf<LLVM_AnyPointer>,
- LLVM_ScalarOrVectorOf<AnySignlessInteger>>;
+ LLVM_ScalarOrVectorOf<AnySignlessInteger>> {
+ let hasFolder = 1;
+}
def LLVM_SExtOp : LLVM_CastOp<"sext", "SExt",
LLVM_ScalarOrVectorOf<AnySignlessInteger>,
LLVM_ScalarOrVectorOf<AnySignlessInteger>> {
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index c7528c970a4ba..74745752fd3dd 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -3562,6 +3562,35 @@ LogicalResult LLVM::BitcastOp::verify() {
return success();
}
+//===----------------------------------------------------------------------===//
+// IntToPtrOp
+//===----------------------------------------------------------------------===//
+
+template <typename CastOp, typename ReverseCastOp>
+static OpFoldResult foldReversibleCast(CastOp castOp) {
+ auto reverseCastOp = castOp.getArg().template getDefiningOp<ReverseCastOp>();
+ if (!reverseCastOp)
+ return {};
+
+ // cast(reverse_cast(x)) -> x
+ if (reverseCastOp.getArg().getType() == castOp.getType())
+ return reverseCastOp.getArg();
+
+ return {};
+}
+
+OpFoldResult LLVM::IntToPtrOp::fold(FoldAdaptor adaptor) {
+ return foldReversibleCast<LLVM::IntToPtrOp, LLVM::PtrToIntOp>(*this);
+}
+
+//===----------------------------------------------------------------------===//
+// PtrToIntOp
+//===----------------------------------------------------------------------===//
+
+OpFoldResult LLVM::PtrToIntOp::fold(FoldAdaptor adaptor) {
+ return foldReversibleCast<LLVM::PtrToIntOp, LLVM::IntToPtrOp>(*this);
+}
+
//===----------------------------------------------------------------------===//
// Folder for LLVM::AddrSpaceCastOp
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/Dialect/LLVMIR/canonicalize.mlir b/mlir/test/Dialect/LLVMIR/canonicalize.mlir
index 8accf6e263863..51f583868aee2 100644
--- a/mlir/test/Dialect/LLVMIR/canonicalize.mlir
+++ b/mlir/test/Dialect/LLVMIR/canonicalize.mlir
@@ -103,10 +103,10 @@ llvm.func @fold_unrelated_extractvalue(%arr: !llvm.array<4 x f32>) -> f32 {
// -----
// CHECK-LABEL: fold_extract_extractvalue
llvm.func @fold_extract_extractvalue(%arr: !llvm.struct<(i64, array<1 x ptr<1>>)>) -> !llvm.ptr<1> {
- // CHECK: llvm.extractvalue %{{.*}}[1, 0]
+ // CHECK: llvm.extractvalue %{{.*}}[1, 0]
// CHECK-NOT: extractvalue
- %a = llvm.extractvalue %arr[1] : !llvm.struct<(i64, array<1 x ptr<1>>)>
- %b = llvm.extractvalue %a[0] : !llvm.array<1 x ptr<1>>
+ %a = llvm.extractvalue %arr[1] : !llvm.struct<(i64, array<1 x ptr<1>>)>
+ %b = llvm.extractvalue %a[0] : !llvm.array<1 x ptr<1>>
llvm.return %b : !llvm.ptr<1>
}
@@ -134,6 +134,32 @@ llvm.func @fold_extract_splat() -> f64 {
// -----
+// CHECK-LABEL: fold_inttoptr_ptrtoint
+// CHECK-SAME: %[[ARG:.+]]: i32) -> i32
+// CHECK-NOT: inttoptr
+// CHECK-NOT: ptrtoint
+// CHECK-NEXT: llvm.return %[[ARG]]
+llvm.func @fold_inttoptr_ptrtoint(%x : i32) -> i32 {
+ %c = llvm.inttoptr %x : i32 to !llvm.ptr
+ %d = llvm.ptrtoint %c : !llvm.ptr to i32
+ llvm.return %d : i32
+}
+
+// -----
+
+// CHECK-LABEL: fold_ptrtoint_inttoptr
+// CHECK-SAME: %[[ARG:.+]]: !llvm.ptr<3>) -> !llvm.ptr<3>
+// CHECK-NOT: inttoptr
+// CHECK-NOT: ptrtoint
+// CHECK-NEXT: llvm.return %[[ARG]]
+llvm.func @fold_ptrtoint_inttoptr(%x : !llvm.ptr<3>) -> !llvm.ptr<3> {
+ %c = llvm.ptrtoint %x : !llvm.ptr<3> to i32
+ %d = llvm.inttoptr %c : i32 to !llvm.ptr<3>
+ llvm.return %d : !llvm.ptr<3>
+}
+
+// -----
+
// CHECK-LABEL: fold_bitcast
// CHECK-SAME: %[[ARG:[[:alnum:]]+]]
// CHECK-NEXT: llvm.return %[[ARG]]
|
| llvm.func @fold_ptrtoint_inttoptr(%x : !llvm.ptr<3>) -> !llvm.ptr<3> { | ||
| %c = llvm.ptrtoint %x : !llvm.ptr<3> to i32 | ||
| %d = llvm.inttoptr %c : i32 to !llvm.ptr<3> | ||
| llvm.return %d : !llvm.ptr<3> |
There was a problem hiding this comment.
I believe this should not be folded since the ptrtoint operation truncates the pointer value to an i32, if I understand correctly? After the folding all bits of the function argument are forwarded to the result.
Do you know what folders LLVM proper implements? I suspect there are some edge cases around these operations that require special handling.
There was a problem hiding this comment.
In particular, I'm not sure about how the provenance is supposed to be handled for this kind of things. I try these example with InstCombine first. Also maybe see what Alive is thinking about these?
There was a problem hiding this comment.
Sorry, that's me sending PRs late at night :)
Thanks for pointing that out. Yes, that folding is not correct unless the size of the pointer is 32 bits.
This is what InstCombine does:
define ptr @test_i32(ptr %p1) {
%i = ptrtoint ptr %p1 to i32
%p2 = inttoptr i32 %i to ptr
ret ptr %p2
}
define ptr @test_i64(ptr %p1) {
%i = ptrtoint ptr %p1 to i64
%p2 = inttoptr i64 %i to ptr
ret ptr %p2
}
-->
define ptr @test_i32(ptr %p1) {
%1 = ptrtoint ptr %p1 to i64
%2 = and i64 %1, 4294967295
%p2 = inttoptr i64 %2 to ptr
ret ptr %p2
}
define ptr @test_i64(ptr %p1) {
ret ptr %p1
}
That makes sense as LLVM defaults to 64-bit pointers. If we add 32-bit pointer info:
target datalayout = "e-p:32:32"
-->
define ptr @test_i32(ptr %p1) {
ret ptr %p1
}
define ptr @test_i64(ptr %p1) {
ret ptr %p1
}
So I think we should only apply these folders if we are able to retrieve the pointer size from the DL info and we can prove that no actual data truncation happens. Questions:
- Is retrieving DL info allowed withing a folder?
- Have we agreed upon how to represent pointer information in the DL for the LLVM dialect already (@rengolin)?
Thanks!
There was a problem hiding this comment.
Also, I don't think this fold is allowed for non-integral pointers
There was a problem hiding this comment.
Is retrieving DL info allowed withing a folder?
I am not aware of a specific rule but I would not recommend to query the data layout from a folder. For such a transformation, a separate optimization pass sounds like the better choice? That way the data layout doesn't need to queried for every fold.
Have we agreed upon how to represent pointer information in the DL for the LLVM dialect already (@rengolin)?
AFAIK the pointer bit width is available today but I don't think non-integral pointer info that @krzysz00 mentioned is available.
There was a problem hiding this comment.
Ok, I can definitely turn this into a pass and see how it goes. Let's do that. Thanks for the suggestion!
This PR is a follow-up to llvm#141891. It introduces a pass that can fold `inttoptr(ptrtoint(x)) -> x` and `ptrtoint(inttoptr(x)) -> x`. The pass takes in a list of address space bitwidths and makes sure that the folding is applied only when it's safe.
|
Pass implementation: #143066 |
This PR adds folders for
inttoptr(ptrtoint(x))andptrtoint(inttoptr(x)).