[CodeGenPrepare] Don't create inttoptr for ni ptrs

[loladiro]

Summary

Arguably non-integral pointers probably shouldn't show up here at all,
but since the backend doesn't complain and this takes valid (according
to the Verifier) IR and makes it invalid, make sure not to introduce
any inttoptr instructions if we're dealing with non-integral pointers.

[loladiro]

Bump. @sanjoy does this look good to you?

[loladiro]

Bump on review for this.

[yuyichao]

I'm seeing a similar failure caused by the same code even with this patch applied on LLVM 4.0.

The relevant IR is

%32 = call %jl_value_t addrspace(10)* %31(i64 %24), !dbg !22697
%34 = addrspacecast %jl_value_t addrspace(10)* %32 to %jl_value_t*
%35 = ptrtoint %jl_value_t* %34 to i64, !dbg !22699
%36 = add i64 %35, 8, !dbg !22699
%76 = inttoptr i64 %36 to i8*
%77 = getelementptr i8, i8* %76, i64 2, !dbg !22717

And when it enters the else branch around L4368 Addr is %77 = getelementptr i8, i8* %76, i64 2, !dbg !22717 and AddrMode.BaseReg is %32 = call %jl_value_t addrspace(10)* %31(i64 %24), !dbg !22697. This is not handled by the non-integral pointer check added in the PR and causes a ptrtoint on %32. The following patch seems to fix it

diff
diff --git a/lib/CodeGen/CodeGenPrepare.cpp b/lib/CodeGen/CodeGenPrepare.cpp
index 44d6b3e264c..4177819e989 100644
--- a/lib/CodeGen/CodeGenPrepare.cpp
+++ b/lib/CodeGen/CodeGenPrepare.cpp
@@ -4067,6 +4067,23 @@ bool CodeGenPrepare::optimizeMemoryInst(Instruction *MemoryInst, Value *Addr,
     // non-integral pointers, so in that case bail out now.
     if (DL->isNonIntegralPointerType(Addr->getType()))
       return false;
+    if (AddrMode.BaseReg) {
+      Type *BaseTy = AddrMode.BaseReg->getType();
+      if (BaseTy->isPointerTy() && DL->isNonIntegralPointerType(BaseTy)) {
+        return false;
+      }
+    }
+    if (AddrMode.Scale) {
+      Type *ScaleTy = AddrMode.ScaledReg->getType();
+      if (ScaleTy->isPointerTy() && DL->isNonIntegralPointerType(ScaleTy)) {
+        return false;
+      }
+    }
+    if (AddrMode.BaseGV) {
+      if (DL->isNonIntegralPointerType(AddrMode.BaseGV->getType())) {
+        return false;
+      }
+    }
 
     DEBUG(dbgs() << "CGP: SINKING nonlocal addrmode: " << AddrMode << " for "
                  << *MemoryInst << "\n");

Not sure if it is the right fix.

[sanjoy]

lgtm

llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp

if (AddrMode.BaseGV) {
         return false;
 
       ResultPtr = AddrMode.BaseGV;
     }
 
     // If the real base value actually came from an inttoptr, then the matcher
     // will look through it and provide only the integer value. In that case,
     // use it here.
+    if (!DL->isNonIntegralPointerType(Addr->getType())) {
       if (!ResultPtr && AddrMode.BaseReg) {
-      ResultPtr =
-        Builder.CreateIntToPtr(AddrMode.BaseReg, Addr->getType(), "sunkaddr");
+        ResultPtr = Builder.CreateIntToPtr(AddrMode.BaseReg, Addr->getType(),
+                                           "sunkaddr");
         AddrMode.BaseReg = nullptr;
       } else if (!ResultPtr && AddrMode.Scale == 1) {
-      ResultPtr =
-        Builder.CreateIntToPtr(AddrMode.ScaledReg, Addr->getType(), "sunkaddr");
+        ResultPtr = Builder.CreateIntToPtr(AddrMode.ScaledReg, Addr->getType(),
+                                           "sunkaddr");
         AddrMode.Scale = 0;
       }
+    }
 
     if (!ResultPtr &&
         !AddrMode.BaseReg && !AddrMode.Scale && !AddrMode.BaseOffs) {
       SunkAddr = Constant::getNullValue(Addr->getType());
     } else if (!ResultPtr) {
       return false;
     } else {
       Type *I8PtrTy =
if (!ResultPtr &&
           ResultPtr = Builder.CreatePointerCast(ResultPtr, I8PtrTy);
         SunkAddr = Builder.CreateGEP(I8Ty, ResultPtr, ResultIndex, "sunkaddr");
       }
 
       if (SunkAddr->getType() != Addr->getType())
         SunkAddr = Builder.CreatePointerCast(SunkAddr, Addr->getType());
     }
   } else {
+    // We'd require a ptrtoint/inttoptr down the line, which we can't do for
+    // non-integral pointers, so in that case bail out now.
+    Type *BaseTy = AddrMode.BaseReg ? AddrMode.BaseReg->getType() : nullptr;
+    Type *ScaleTy = AddrMode.Scale ? AddrMode.ScaledReg->getType() : nullptr;
+    PointerType *BasePtrTy = dyn_cast_or_null<PointerType>(BaseTy);
+    PointerType *ScalePtrTy = dyn_cast_or_null<PointerType>(ScaleTy);
+    if (DL->isNonIntegralPointerType(Addr->getType()) ||
+        (BasePtrTy && DL->isNonIntegralPointerType(BasePtrTy)) ||
+        (ScalePtrTy && DL->isNonIntegralPointerType(ScalePtrTy)) ||
+        (AddrMode.BaseGV &&
+         DL->isNonIntegralPointerType(AddrMode.BaseGV->getType())))
+      return false;
+
     DEBUG(dbgs() << "CGP: SINKING nonlocal addrmode: " << AddrMode << " for "
                  << *MemoryInst << "\n");
     Type *IntPtrTy = DL->getIntPtrType(Addr->getType());
     Value *Result = nullptr;
 
     // Start with the base register. Do this first so that subsequent address
     // matching finds it last, which will prevent it from trying to match it
     // as the scaled value in case it happens to be a mul. That would be

llvm/trunk/test/Transforms/CodeGenPrepare/nonintegral.ll

+; RUN: opt -S -codegenprepare < %s | FileCheck %s
+; RUN: opt -S -codegenprepare -addr-sink-using-gep=false < %s | FileCheck %s
+
+; This target data layout is modified to have a non-integral addrspace(1),
+; in order to verify that codegenprepare does not try to introduce illegal
+; inttoptrs.
+target datalayout =
+"e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-ni:1"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @test_simple(i1 %cond, i64 addrspace(1)* %base) {
+; CHECK-LABEL: @test_simple
+; CHECK-NOT: inttoptr {{.*}} to i64 addrspace(1)*
+entry:
+  %addr = getelementptr inbounds i64, i64 addrspace(1)* %base, i64 5
+  %casted = bitcast i64 addrspace(1)* %addr to i32 addrspace(1)*
+  br i1 %cond, label %if.then, label %fallthrough
+
+if.then:
+  %v = load i32, i32 addrspace(1)* %casted, align 4
+  br label %fallthrough
+
+fallthrough:
+  ret void
+}
+
+
+define void @test_inttoptr_base(i1 %cond, i64 %base) {
+; CHECK-LABEL: @test_inttoptr_base
+; CHECK-NOT: inttoptr {{.*}} to i64 addrspace(1)*
+entry:
+; Doing the inttoptr in the integral addrspace(0) followed by an explicit
+; (frontend-introduced) addrspacecast is fine. We cannot however introduce
+; a direct inttoptr to addrspace(1)
+  %baseptr = inttoptr i64 %base to i64*
+  %baseptrni = addrspacecast i64 *%baseptr to i64 addrspace(1)*
+  %addr = getelementptr inbounds i64, i64 addrspace(1)* %baseptrni, i64 5
+  %casted = bitcast i64 addrspace(1)* %addr to i32 addrspace(1)*
+  br i1 %cond, label %if.then, label %fallthrough
+
+if.then:
+  %v = load i32, i32 addrspace(1)* %casted, align 4
+  br label %fallthrough
+
+fallthrough:
+  ret void
+}
+
+define void @test_ptrtoint_base(i1 %cond, i64 addrspace(1)* %base) {
+; CHECK-LABEL: @test_ptrtoint_base
+; CHECK-NOT: ptrtoint addrspace(1)* {{.*}} to i64
+entry:
+; This one is inserted by the frontend, so it's fine. We're not allowed to
+; directly ptrtoint %base ourselves though
+  %baseptr0 = addrspacecast i64 addrspace(1)* %base to i64*
+  %toint = ptrtoint i64* %baseptr0 to i64
+  %added = add i64 %toint, 8
+  %toptr = inttoptr i64 %added to i64*
+  %geped = getelementptr i64, i64* %toptr, i64 2
+  br i1 %cond, label %if.then, label %fallthrough
+
+if.then:
+  %v = load i64, i64* %geped, align 4
+  br label %fallthrough
+
+fallthrough:
+  ret void
+}