[InstCombine] Fold IntToPtr/PtrToInt to bitcast

[Krishnakariya]

Summary

The inttoptr/ptrtoint roundtrip optimization is not always correct. We are working towards removing this optimization and adding support to specific cases where this optimization works. This patch is the first one on this line.

Consider the example,

%i = ptrtoint i8* %X to i64
%p = inttoptr i64 %i to i16*   
%cmp = icmp eq i8* %load, %p

In this specific case, the inttoptr/ptrtoint optimization is correct as it only compares the pointers. In this patch, we fold inttoptr/ptrtoint to a bitcast (if src and dest types are different).

[efriedma]

I think you need to check the type of the result of the ptrtoint. (instcombine will normally canonicalize to have the same width as the pointer type, but that isn't guaranteed to happen before this code runs.)

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

4595

Might as well call CreateBitCast unconditionally; it has the same bailout internally.

[Krishnakariya]

Added the check and test case for the same.

[efriedma]

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

4593

I think you also need to check the size of p1? If the pointers are in different address spaces, the sizes might not match, so the bitcast will fail to verify. Again, maybe hard to trigger in practice because the ptrtoint destination type gets canonicalized.

4597

Minor nit: Value *NewOp0 = Builder.CreateBitCast(P, Op0Ty);

[Krishnakariya]

Updated the check.

[efriedma]

LGTM

[nikic]

As this fold is currently subsumed by the more general inttoptr(ptrtoint(x)) fold, your test isn't really testing anything useful right now. Even without your added code, it folds to the same result.

I think you should add an option that disables the primary fold, and then use that option in the test. That way you can show that this particular case is still handled even once the general fold has been removed.

[Krishnakariya]

Added an command-line option disable-i2p-p2i-opt to opt to disable inttoptr/ptrtoint optimization.
I have removed the const keyword from the declaration of the CastResults array.
This change is temporary, and the array would be constified again.

[nikic]

llvm/lib/IR/Instructions.cpp

2798 ↗

(On Diff #357492)

It would be better to modify case 7 below instead.

[Krishnakariya]

llvm/lib/IR/Instructions.cpp

2798 ↗

(On Diff #357492)

Thanks, I've updated the patch.

[nikic]

It would be good to pre-commit the tests, so the diff is clearly visible.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

4588

The code in isEliminableCastPair() also checks that the address spaces match. Can you please add tests involving different address spaces?

[Krishnakariya]

Created a new function (simplifyIntToPtrRoundTripCast) which simplifies the inttoptr/ptrtoint cast.

[xbolva00]

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

4577

... and then this code can be simplified...

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

354

return nullptr instead Val? ...

[Krishnakariya]

simplified code for function "foldICmpWithCastOp".

[xbolva00]

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

4577

if (Op0 || Op1)

4578

return new ICmpInst(ICmp.getPredicate(), Op0 ? Op0 : ICmp.getOperand(0), Op1 ? Op1 : ICmp.getOperand(1));

I think you should use NewOp0 or SimplifiedO0 or something like that.

[nikic]

LGTM

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

4574

Maybe add a brief comment here that explains why it is legal? Just "because icmp doesn't care about provenance" should do.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

352

You could directly cast to IntToPtrInst here.

[Krishnakariya]

Simplified code.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

static Instruction *foldICmpWithZextOrSext(ICmpInst &ICmp,
   // Is source op negative?
   // icmp ugt (sext X), C --> icmp slt X, 0
   assert(ICmp.getPredicate() == ICmpInst::ICMP_UGT && "ICmp should be folded!");
   return new ICmpInst(CmpInst::ICMP_SLT, X, Constant::getNullValue(SrcTy));
 }
 
 /// Handle icmp (cast x), (cast or constant).
 Instruction *InstCombinerImpl::foldICmpWithCastOp(ICmpInst &ICmp) {
+  // If any operand of ICmp is a inttoptr roundtrip cast then remove it as
+  // icmp compares only pointer's value.
+  // icmp (inttoptr (ptrtoint p1)), p2 --> icmp p1, p2.
+  Value *SimplifiedOp0 = simplifyIntToPtrRoundTripCast(ICmp.getOperand(0));
+  Value *SimplifiedOp1 = simplifyIntToPtrRoundTripCast(ICmp.getOperand(1));
+  if (SimplifiedOp0 || SimplifiedOp1)
+    return new ICmpInst(ICmp.getPredicate(),
+                        SimplifiedOp0 ? SimplifiedOp0 : ICmp.getOperand(0),
+                        SimplifiedOp1 ? SimplifiedOp1 : ICmp.getOperand(1));
+
   auto *CastOp0 = dyn_cast<CastInst>(ICmp.getOperand(0));
   if (!CastOp0)
     return nullptr;
   if (!isa<Constant>(ICmp.getOperand(1)) && !isa<CastInst>(ICmp.getOperand(1)))
     return nullptr;
 
   Value *Op0Src = CastOp0->getOperand(0);
   Type *SrcTy = CastOp0->getSrcTy();
   Type *DestTy = CastOp0->getDestTy();
 
   // Turn icmp (ptrtoint x), (ptrtoint/c) into a compare of the input if the
   // integer type is the same size as the pointer type.
   auto CompatibleSizes = [&](Type *SrcTy, Type *DestTy) {
     if (isa<VectorType>(SrcTy)) {
       SrcTy = cast<VectorType>(SrcTy)->getElementType();
       DestTy = cast<VectorType>(DestTy)->getElementType();
     }
     return DL.getPointerTypeSizeInBits(SrcTy) == DestTy->getIntegerBitWidth();
   };
   if (CastOp0->getOpcode() == Instruction::PtrToInt &&
       CompatibleSizes(SrcTy, DestTy)) {
     Value *NewOp1 = nullptr;

llvm/lib/Transforms/InstCombine/InstCombineInternal.h

private:
   ///
   /// \return 0 if the cast pair cannot be eliminated, otherwise returns an
   /// Instruction::CastOps value for a cast that can replace the pair, casting
   /// CI1->getSrcTy() to CI2->getDstTy().
   ///
   /// \see CastInst::isEliminableCastPair
   Instruction::CastOps isEliminableCastPair(const CastInst *CI1,
                                             const CastInst *CI2);
+  Value *simplifyIntToPtrRoundTripCast(Value *Val);
 
   Value *foldAndOfICmps(ICmpInst *LHS, ICmpInst *RHS, BinaryOperator &And);
   Value *foldOrOfICmps(ICmpInst *LHS, ICmpInst *RHS, BinaryOperator &Or);
   Value *foldXorOfICmps(ICmpInst *LHS, ICmpInst *RHS, BinaryOperator &Xor);
 
   /// Optimize (fcmp)&(fcmp) or (fcmp)|(fcmp).
   /// NOTE: Unlike most of instcombine, this returns a Value which should
   /// already be inserted into the function.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

static bool simplifyAssocCastAssoc(BinaryOperator *BinOp1,
   Type *DestTy = C1->getType();
   Constant *CastC2 = ConstantExpr::getCast(CastOpcode, C2, DestTy);
   Constant *FoldedC = ConstantExpr::get(AssocOpcode, C1, CastC2);
   IC.replaceOperand(*Cast, 0, BinOp2->getOperand(0));
   IC.replaceOperand(*BinOp1, 1, FoldedC);
   return true;
 }
 
+// Simplifies IntToPtr/PtrToInt RoundTrip Cast To BitCast.
+// inttoptr ( ptrtoint (x) ) --> x
+Value *InstCombinerImpl::simplifyIntToPtrRoundTripCast(Value *Val) {
+  auto *IntToPtr = dyn_cast<IntToPtrInst>(Val);
+  if (IntToPtr && DL.getPointerTypeSizeInBits(IntToPtr->getDestTy()) ==
+                      DL.getTypeSizeInBits(IntToPtr->getSrcTy())) {
+    auto *PtrToInt = dyn_cast<PtrToIntInst>(IntToPtr->getOperand(0));
+    Type *CastTy = IntToPtr->getDestTy();
+    if (PtrToInt &&
+        CastTy->getPointerAddressSpace() ==
+            PtrToInt->getSrcTy()->getPointerAddressSpace() &&
+        DL.getPointerTypeSizeInBits(PtrToInt->getSrcTy()) ==
+            DL.getTypeSizeInBits(PtrToInt->getDestTy())) {
+      return Builder.CreateBitCast(PtrToInt->getOperand(0), CastTy);
+    }
+  }
+  return nullptr;
+}
+
 /// This performs a few simplifications for operators that are associative or
 /// commutative:
 ///
 ///  Commutative operators:
 ///
 ///  1. Order operands such that they are listed from right (least complex) to
 ///     left (most complex).  This puts constants before unary operators before
 ///     binary operators.

llvm/test/Transforms/InstCombine/ptr-int-ptr-icmp.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -instcombine -S -disable-i2p-p2i-opt < %s | FileCheck %s
 
 target datalayout = "e-p:64:64-p1:16:16-p2:32:32:32-p3:64:64:64"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; icmp (inttoptr (ptrtoint p1)), p2 --> icmp p1, p2.
 
 define i1 @func(i8* %X, i8* %Y) {
 ; CHECK-LABEL: @func(
-; CHECK-NEXT:    [[I:%.*]] = ptrtoint i8* [[X:%.*]] to i64
-; CHECK-NEXT:    [[P:%.*]] = inttoptr i64 [[I]] to i8*
-; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8* [[P]], [[Y:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8* [[X:%.*]], [[Y:%.*]]
 ; CHECK-NEXT:    ret i1 [[CMP]]
 ;
   %i = ptrtoint i8* %X to i64
   %p = inttoptr i64 %i to i8*
   %cmp = icmp eq i8* %p, %Y
   ret i1 %cmp
 }
 
 define i1 @func_pointer_different_types(i16* %X, i8* %Y) {
 ; CHECK-LABEL: @func_pointer_different_types(
-; CHECK-NEXT:    [[I:%.*]] = ptrtoint i16* [[X:%.*]] to i64
-; CHECK-NEXT:    [[P:%.*]] = inttoptr i64 [[I]] to i8*
-; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8* [[P]], [[Y:%.*]]
+; CHECK-NEXT:    [[TMP1:%.*]] = bitcast i16* [[X:%.*]] to i8*
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8* [[TMP1]], [[Y:%.*]]
 ; CHECK-NEXT:    ret i1 [[CMP]]
 ;
   %i = ptrtoint i16* %X to i64
   %p = inttoptr i64 %i to i8*
   %cmp = icmp eq i8* %p, %Y
   ret i1 %cmp
 }
 
 declare i8* @gen8ptr()
 
 define i1 @func_commutative(i16* %X) {
 ; CHECK-LABEL: @func_commutative(
 ; CHECK-NEXT:    [[Y:%.*]] = call i8* @gen8ptr()
-; CHECK-NEXT:    [[I:%.*]] = ptrtoint i16* [[X:%.*]] to i64
-; CHECK-NEXT:    [[P:%.*]] = inttoptr i64 [[I]] to i8*
-; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8* [[Y]], [[P]]
+; CHECK-NEXT:    [[TMP1:%.*]] = bitcast i16* [[X:%.*]] to i8*
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8* [[Y]], [[TMP1]]
 ; CHECK-NEXT:    ret i1 [[CMP]]
 ;
   %Y = call i8* @gen8ptr() ; thwart complexity-based canonicalization
   %i = ptrtoint i16* %X to i64
   %p = inttoptr i64 %i to i8*
   %cmp = icmp eq i8* %Y, %p
   ret i1 %cmp
 }