Fix invalid ptrtoint in InstCombine

[yuyichao]

Summary

A few things I'm not sure about.

1. Is this the right place to do it? (It seems that this is used in both instcombine and jumpthreading with a similar pattern but I'm not sure if this can be triggerred in jumpthreading)
2. Do I need the address space check? I can't find a different place that checks the address space matching and the caller does not try addrspacecast (and probably shouldn't) though I also can't trigger the code with two address space ....

[loladiro]

lib/Analysis/Loads.cpp

373

I'm a bit confused by this. Isn't Ptr the thing we're loading from? Why are we comparing it against LI->getType() shouldn't we be looking at AccessTy?

[yuyichao]

Use AccessTy and added a test to make sure that the case where we load from an non-integral point is still optimized.

[yuyichao]

Ping?

[majnemer]

lib/Analysis/Loads.cpp

375–385

Please fix the formatting.

407–418

Ditto.

[majnemer]

Your code has no comments. At a glance, I have no idea what it is trying to accomplish.

[yuyichao]

There was no comment since the only comment I can add would be repeating the condition itself. I do like to add comment about why this is needed or if this is the only way we can fix the issue but that's also what I'm not sure about (quesiton 1 in my review comment). Anyway, I added a comment to repeat the condition for now.

[yuyichao]

I've found another failure in instcombine (see last test) so it seems to me that CastInst::isBitOrNoopPointerCastable is the right place for this logic. This also answers my second question since the cast between address spaces is already handled by isBitCastable

[yuyichao]

Ping.

[yuyichao]

Ping.

[sanjoy]

LGTM

[yuyichao]

Thanks for the review.

lib/Analysis/Loads.cpp

if (LoadInst *LI = dyn_cast<LoadInst>(Inst))
               LI->getPointerOperand()->stripPointerCasts(), StrippedPtr) &&
           CastInst::isBitOrNoopPointerCastable(LI->getType(), AccessTy, DL)) {
 
         // We can value forward from an atomic to a non-atomic, but not the
         // other way around.
         if (LI->isAtomic() < AtLeastAtomic)
           return nullptr;
 
+        if (Ptr->getType()->isPointerTy()) {
+          PointerType *Ty1 = cast<PointerType>(Ptr->getType());
+          if (PointerType *Ty2 = dyn_cast<PointerType>(LI->getType())) {
+            if (Ty1->getAddressSpace() != Ty2->getAddressSpace()) {
+              return nullptr;
+            }
+          }
+          else if (DL.isNonIntegralPointerType(Ty1)) {
+            return nullptr;
+          }
+        }
+        else if (DL.isNonIntegralPointerType(LI->getType())) {
+          return nullptr;
+        }
+
         if (IsLoadCSE)
             *IsLoadCSE = true;
         return LI;
       }
 
     if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
       Value *StorePtr = SI->getPointerOperand()->stripPointerCasts();
       // If this is a store through Ptr, the value is available!
       // (This is true even if the store is volatile or atomic, although
       // those cases are unlikely.)
       if (AreEquivalentAddressValues(StorePtr, StrippedPtr) &&
           CastInst::isBitOrNoopPointerCastable(SI->getValueOperand()->getType(),
                                                AccessTy, DL)) {
 
         // We can value forward from an atomic to a non-atomic, but not the
         // other way around.
         if (SI->isAtomic() < AtLeastAtomic)
           return nullptr;
 
+        if (Ptr->getType()->isPointerTy()) {
+          PointerType *Ty1 = cast<PointerType>(Ptr->getType());
+          if (PointerType *Ty2 = dyn_cast<PointerType>(SI->getValueOperand()->getType())) {
+            if (Ty1->getAddressSpace() != Ty2->getAddressSpace()) {
+              return nullptr;
+            }
+          }
+          else if (DL.isNonIntegralPointerType(Ty1)) {
+            return nullptr;
+          }
+        }
+        else if (DL.isNonIntegralPointerType(SI->getValueOperand()->getType())) {
+          return nullptr;
+        }
+
         if (IsLoadCSE)
           *IsLoadCSE = false;
         return SI->getOperand(0);
       }
 
       // If both StrippedPtr and StorePtr reach all the way to an alloca or
       // global and they are different, ignore the store. This is a trivial form
       // of alias analysis that is important for reg2mem'd code.

test/Transforms/InstCombine/non-integral-pointers.ll

 ; CHECK-LABEL: @f_3(
 entry:
 ; CHECK: load i64
 ; CHECK:  store i64
   %val = load i8 addrspace(3)*, i8 addrspace(3)** %ptr0
   store i8 addrspace(3)* %val, i8 addrspace(3)** %ptr1
   ret void
 }
+
+define i64 @g(i8 addrspace(4)** %gp) {
+; CHECK-LABEL: @g(
+  %.pre = load i8 addrspace(4)*, i8 addrspace(4)** %gp, align 8
+  %v74 = call i8 addrspace(4)* @alloc()
+  %v75 = addrspacecast i8 addrspace(4)* %v74 to i8*
+  %v76 = bitcast i8* %v75 to i8 addrspace(4)**
+  %v77 = getelementptr i8 addrspace(4)*, i8 addrspace(4)** %v76, i64 -1
+  store i8 addrspace(4)* %.pre, i8 addrspace(4)** %v77, align 8
+  %v78 = bitcast i8 addrspace(4)* %v74 to i8 addrspace(4)* addrspace(4)*
+  %v79 = addrspacecast i8 addrspace(4)* addrspace(4)* %v78 to i8 addrspace(4)**
+  %v80 = bitcast i8 addrspace(4)** %v77 to i64*
+; CHECK-NOT: ptrtoint
+  %v81 = load i64, i64* %v80, align 8
+  ret i64 %v81
+}
+
+declare i8 addrspace(4)* @alloc()