[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

4550

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

4548

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.

4552

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

It would be better to modify case 7 below instead.

[Krishnakariya]

llvm/lib/IR/Instructions.cpp

2798

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

4543

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

4542

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

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

354 ↗

(On Diff #358674)

return nullptr instead Val? ...

[Krishnakariya]

simplified code for function "foldICmpWithCastOp".

[xbolva00]

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

4542

if (Op0 || Op1)

4543

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

4539

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 ↗

(On Diff #358928)

You could directly cast to IntToPtrInst here.

[Krishnakariya]

Simplified code.

llvm/lib/IR/Instructions.cpp

 #include "llvm/Support/TypeSize.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <vector>
 
 using namespace llvm;
 
+static cl::opt<bool> DisableI2pP2iOpt(
+    "disable-i2p-p2i-opt", cl::init(false),
+    cl::desc("Disables inttoptr/ptrtoint roundtrip optimization"));
+
 //===----------------------------------------------------------------------===//
 //                            AllocaInst Class
 //===----------------------------------------------------------------------===//
 
 Optional<TypeSize>
 AllocaInst::getAllocationSizeInBits(const DataLayout &DL) const {
   TypeSize Size = DL.getTypeAllocSizeInBits(getAllocatedType());
   if (isArrayAllocation()) {
unsigned CastInst::isEliminableCastPair(
   // For example, we could merge "fptoui double to i32" + "zext i32 to i64",
   // into "fptoui double to i64", but this loses information about the range
   // of the produced value (we no longer know the top-part is all zeros).
   // Further this conversion is often much more expensive for typical hardware,
   // and causes issues when building libgcc.  We disallow fptosi+sext for the
   // same reason.
   const unsigned numCastOps =
     Instruction::CastOpsEnd - Instruction::CastOpsBegin;
-  static const uint8_t CastResults[numCastOps][numCastOps] = {
+  static uint8_t CastResults[numCastOps][numCastOps] = {
     // T        F  F  U  S  F  F  P  I  B  A  -+
     // R  Z  S  P  P  I  I  T  P  2  N  T  S   |
     // U  E  E  2  2  2  2  R  E  I  T  C  C   +- secondOp
     // N  X  X  U  S  F  F  N  X  N  2  V  V   |
     // C  T  T  I  I  P  P  C  T  T  P  T  T  -+
     {  1, 0, 0,99,99, 0, 0,99,99,99, 0, 3, 0}, // Trunc         -+
     {  8, 1, 9,99,99, 2,17,99,99,99, 2, 3, 0}, // ZExt           |
     {  8, 0, 1,99,99, 0, 2,99,99,99, 0, 3, 0}, // SExt           |
     {  0, 0, 0,99,99, 0, 0,99,99,99, 0, 3, 0}, // FPToUI         |
     {  0, 0, 0,99,99, 0, 0,99,99,99, 0, 3, 0}, // FPToSI         |
     { 99,99,99, 0, 0,99,99, 0, 0,99,99, 4, 0}, // UIToFP         +- firstOp
     { 99,99,99, 0, 0,99,99, 0, 0,99,99, 4, 0}, // SIToFP         |
     { 99,99,99, 0, 0,99,99, 0, 0,99,99, 4, 0}, // FPTrunc        |
     { 99,99,99, 2, 2,99,99, 8, 2,99,99, 4, 0}, // FPExt          |
     {  1, 0, 0,99,99, 0, 0,99,99,99, 7, 3, 0}, // PtrToInt       |
     { 99,99,99,99,99,99,99,99,99,11,99,15, 0}, // IntToPtr       |
     {  5, 5, 5, 6, 6, 5, 5, 6, 6,16, 5, 1,14}, // BitCast        |
     {  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,13,12}, // AddrSpaceCast -+
   };
 
+  // Disable inttoptr/ptrtoint optimization if enabled.
+  if (DisableI2pP2iOpt)
+    CastResults[9][10] = 0;
   // TODO: This logic could be encoded into the table above and handled in the
   // switch below.
   // If either of the casts are a bitcast from scalar to vector, disallow the
   // merging. However, any pair of bitcasts are allowed.
   bool IsFirstBitcast  = (firstOp == Instruction::BitCast);
   bool IsSecondBitcast = (secondOp == Instruction::BitCast);
   bool AreBothBitcasts = IsFirstBitcast && IsSecondBitcast;
 

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) {
   auto *CastOp0 = dyn_cast<CastInst>(ICmp.getOperand(0));
   if (!CastOp0)
     return nullptr;
+
+  // icmp (inttoptr (ptrtoint p1)), p2 --> icmp p1, p2.
+  if (CastOp0->getOpcode() == Instruction::IntToPtr &&
+      (DL.getPointerTypeSizeInBits(CastOp0->getDestTy()) ==
+       DL.getTypeSizeInBits(CastOp0->getSrcTy()))) {
+    auto *I = dyn_cast<PtrToIntInst>(CastOp0->getOperand(0));
+    if (I && (DL.getPointerTypeSizeInBits(I->getSrcTy()) ==
+              DL.getTypeSizeInBits(I->getDestTy()))) {
+      Value *P = I->getOperand(0);
+      Type *Op0Ty = CastOp0->getDestTy();
+      Value *NewOp0 = Builder.CreateBitCast(P, Op0Ty);
+      return new ICmpInst(ICmp.getPredicate(), NewOp0, ICmp.getOperand(1));
+    }
+  }
+
   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

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-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+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:    [[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 @func2(i16* %X, i8* %Y) {
+; CHECK-LABEL: @func2(
+; 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
+}
+
+define i1 @func3(i16* %X, i8* %Y) {
+; CHECK-LABEL: @func3(
+; 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* %Y, %p
+  ret i1 %cmp
+}
+
+; Negative test - Wrong Integer type.
+
+define i1 @func4(i16* %X, i8* %Y) {
+; CHECK-LABEL: @func4(
+; CHECK-NEXT:    [[TMP1:%.*]] = ptrtoint i16* [[X:%.*]] to i64
+; CHECK-NEXT:    [[TMP2:%.*]] = and i64 [[TMP1]], 4294967295
+; CHECK-NEXT:    [[P:%.*]] = inttoptr i64 [[TMP2]] to i8*
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8* [[P]], [[Y:%.*]]
+; CHECK-NEXT:    ret i1 [[CMP]]
+;
+  %i = ptrtoint i16* %X to i32
+  %p = inttoptr i32 %i to i8*
+  %cmp = icmp eq i8* %Y, %p
+  ret i1 %cmp
+}