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 ↗

(On Diff #102978)

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

385–395 ↗

(On Diff #108806)

Please fix the formatting.

417–428 ↗

(On Diff #108806)

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.

llvm/trunk/lib/IR/Instructions.cpp

bool CastInst::isBitCastable(Type *SrcTy, Type *DestTy) {
   if (DestTy->isX86_MMXTy() || SrcTy->isX86_MMXTy())
     return false;
 
   return true;
 }
 
 bool CastInst::isBitOrNoopPointerCastable(Type *SrcTy, Type *DestTy,
                                           const DataLayout &DL) {
+  // ptrtoint and inttoptr are not allowed on non-integral pointers
   if (auto *PtrTy = dyn_cast<PointerType>(SrcTy))
     if (auto *IntTy = dyn_cast<IntegerType>(DestTy))
-      return IntTy->getBitWidth() == DL.getPointerTypeSizeInBits(PtrTy);
+      return (IntTy->getBitWidth() == DL.getPointerTypeSizeInBits(PtrTy) &&
+              !DL.isNonIntegralPointerType(PtrTy));
   if (auto *PtrTy = dyn_cast<PointerType>(DestTy))
     if (auto *IntTy = dyn_cast<IntegerType>(SrcTy))
-      return IntTy->getBitWidth() == DL.getPointerTypeSizeInBits(PtrTy);
+      return (IntTy->getBitWidth() == DL.getPointerTypeSizeInBits(PtrTy) &&
+              !DL.isNonIntegralPointerType(PtrTy));
 
   return isBitCastable(SrcTy, DestTy);
 }
 
 // Provide a way to get a "cast" where the cast opcode is inferred from the
 // types and size of the operand. This, basically, is a parallel of the
 // logic in the castIsValid function below.  This axiom should hold:
 //   castIsValid( getCastOpcode(Val, Ty), Val, Ty)

llvm/trunk/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(
+  ; CHECK: load
+  %.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
+  ; CHECK: store
+  store i8 addrspace(4)* %.pre, i8 addrspace(4)** %v77, align 8
+  %v80 = bitcast i8 addrspace(4)** %v77 to i64*
+  ; CHECK: load
+  ; CHECK-NOT: ptrtoint
+  %v81 = load i64, i64* %v80, align 8
+  ret i64 %v81
+}
+
+define i64 @g2(i8* addrspace(4)* %gp) {
+  ; CHECK-LABEL: @g2(
+  ; CHECK: load
+  %.pre = load i8*, i8* addrspace(4)* %gp, align 8
+  %v74 = call i8 addrspace(4)* @alloc()
+  %v76 = bitcast i8 addrspace(4)* %v74 to i8* addrspace(4)*
+  %v77 = getelementptr i8*, i8* addrspace(4)* %v76, i64 -1
+  ; CHECK: store
+  store i8* %.pre, i8* addrspace(4)* %v77, align 8
+  %v80 = bitcast i8* addrspace(4)* %v77 to i64 addrspace(4)*
+  ; CHECK-NOT: store
+  %v81 = load i64, i64 addrspace(4)* %v80, align 8
+  ret i64 %v81
+}
+
+declare i8 addrspace(4)* @alloc()
+
+define i64 @f_4(i8 addrspace(4)* %v0) {
+  ; CHECK-LABEL: @f_4(
+  ; CHECK-NOT: ptrtoint
+  %v5 = bitcast i64 (i64)* @f_5 to i64 (i8 addrspace(4)*)*
+  %v6 = call i64 %v5(i8 addrspace(4)* %v0)
+  ret i64 %v6
+}
+
+declare i64 @f_5(i64)