Don't look through addrspacecast in GetPointerBaseWithConstantOffset

[apilipenko]

Summary

This is similar to D13008

Pointers in different addrspaces can have different sizes, so it's not valid to look through addrspace cast calculating base and offset for a value.

[reames]

lib/Analysis/ValueTracking.cpp

2845 ↗

(On Diff #71832)

For consistency, you should also change getUnderlyingObject at the same time.

test/Transforms/GVN/PRE/rle.ll

321 ↗

(On Diff #71832)

Hm, loosing this optimization seems unfortunate. Any chance we can peak through addrspace casts only when the two pointer sizes are equal?

[apilipenko]

lib/Analysis/ValueTracking.cpp

2845 ↗

(On Diff #71832)

I don't think that we should. According to the description GetUnderlyingObject "strips off any GEP address adjustments and pointer casts from the specified value, returning the original object being addressed".

GetPointerBaseWithConstantOffset:
"Analyze the specified pointer to see if it can be expressed as a base pointer plus a constant offset. Return the base and offset to the caller."

GetUnderlyingObject is just more powerful at looking through casts, because some casts can hinder a base plus an offset representation. The same thing happens to Value::stripPointerCasts and Value::stripAndAccumulateInBoundsConstantOffsets.

test/Transforms/GVN/PRE/rle.ll

321 ↗

(On Diff #71832)

Because addrspaces don't have clearly defined semantic in langref I think we can do this. Although, in this case I'd also go and change Value::stripAndAccumulateInBoundsConstantOffsets to do the same (currently it doesn't look through addrspace casts at all).

[apilipenko]

Let GetPointerBaseWithConstantOffset look through addrspace casts which doesn't change pointer size

[apilipenko]

test/Transforms/GVN/PRE/rle-addrspace-cast.ll

3 ↗

(On Diff #72166)

I had to move this test because original rle.ll test is run with datalayout with different pointer sizes in different address spaces.

[reames]

LGTM w/comment addressed.

test/Transforms/GVN/PRE/rle-addrspace-cast.ll

1 ↗

(On Diff #72166)

You should add a data layout specifier here. I think that we otherwise pick up the default for a given host which might cause this test to spurious fail.

llvm/trunk/lib/Analysis/ValueTracking.cpp

while (Visited.insert(Ptr).second) {
     if (GEPOperator *GEP = dyn_cast<GEPOperator>(Ptr)) {
       APInt GEPOffset(BitWidth, 0);
       if (!GEP->accumulateConstantOffset(DL, GEPOffset))
         break;
 
       ByteOffset += GEPOffset;
 
       Ptr = GEP->getPointerOperand();
-    } else if (Operator::getOpcode(Ptr) == Instruction::BitCast ||
-               Operator::getOpcode(Ptr) == Instruction::AddrSpaceCast) {
+    } else if (Operator::getOpcode(Ptr) == Instruction::BitCast) {
       Ptr = cast<Operator>(Ptr)->getOperand(0);
+    } else if (AddrSpaceCastInst *ASCI = dyn_cast<AddrSpaceCastInst>(Ptr)) {
+      Value *SourcePtr = ASCI->getPointerOperand();
+      // Don't look through addrspace cast which changes pointer size
+      if (BitWidth != DL.getPointerTypeSizeInBits(SourcePtr->getType()))
+        break;
+      Ptr = SourcePtr;
     } else if (GlobalAlias *GA = dyn_cast<GlobalAlias>(Ptr)) {
       if (GA->isInterposable())
         break;
       Ptr = GA->getAliasee();
     } else {
       break;
     }
   }

llvm/trunk/test/Transforms/GVN/PRE/rle-addrspace-cast.ll

+; RUN: opt < %s -default-data-layout="e-p:32:32:32-p1:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-n8:16:32" -basicaa -gvn -S -die | FileCheck %s
+
+define i8 @coerce_offset0_addrspacecast(i32 %V, i32* %P) {
+  store i32 %V, i32* %P
+
+  %P2 = addrspacecast i32* %P to i8 addrspace(1)*
+  %P3 = getelementptr i8, i8 addrspace(1)* %P2, i32 2
+
+  %A = load i8, i8 addrspace(1)* %P3
+  ret i8 %A
+; CHECK-LABEL: @coerce_offset0_addrspacecast(
+; CHECK-NOT: load
+; CHECK: ret i8
+}

llvm/trunk/test/Transforms/GVN/PRE/rle.ll

define i8 @coerce_offset0(i32 %V, i32* %P) {
 
   %A = load i8, i8* %P3
   ret i8 %A
 ; CHECK-LABEL: @coerce_offset0(
 ; CHECK-NOT: load
 ; CHECK: ret i8
 }
 
-define i8 @coerce_offset0_addrspacecast(i32 %V, i32* %P) {
-  store i32 %V, i32* %P
-
-  %P2 = addrspacecast i32* %P to i8 addrspace(1)*
-  %P3 = getelementptr i8, i8 addrspace(1)* %P2, i32 2
-
-  %A = load i8, i8 addrspace(1)* %P3
-  ret i8 %A
-; CHECK-LABEL: @coerce_offset0_addrspacecast(
-; CHECK-NOT: load
-; CHECK: ret i8
-}
-
 ;; non-local i32/float -> i8 load forwarding.
 define i8 @coerce_offset_nonlocal0(i32* %P, i1 %cond) {
   %P2 = bitcast i32* %P to float*
   %P3 = bitcast i32* %P to i8*
   %P4 = getelementptr i8, i8* %P3, i32 2
   br i1 %cond, label %T, label %F
 T:
   store i32 57005, i32* %P

llvm/trunk/test/Transforms/GVN/addrspace-cast.ll

+; RUN: opt < %s -gvn -S | FileCheck %s
+target datalayout = "e-m:e-p:16:16-p1:32:16-i32:16-i64:16-n8:16"
+
+; In cases where two address spaces do not have the same size pointer, the
+; input for the addrspacecast should not be used as a substitute for itself
+; when manipulating the pointer.
+
+; Check that we don't hit the assert in this scenario
+define i8 @test(i32 %V, i32* %P) {
+; CHECK-LABEL: @test(
+; CHECK: load
+  %P1 = getelementptr inbounds i32, i32* %P, i16 16
+
+  store i32 %V, i32* %P1
+
+  %P2 = addrspacecast i32* %P1 to i8 addrspace(1)*
+  %P3 = getelementptr i8, i8 addrspace(1)* %P2, i32 2
+
+  %A = load i8, i8 addrspace(1)* %P3
+  ret i8 %A
+}