Fix assert when inlining a constantexpr addrspacecast

[arsenm]

Summary

The pointer size of the addrspacecasted pointer might not have matched,
so this would have hit an assert in accumulateConstantOffset.

I think this was here to allow constant folding of a load of an
addrspacecasted constant. Accumulating the offset through the
addrspacecast doesn't make much sense, so something else is necessary
to allow folding the load through this cast.

[majnemer]

lib/Analysis/ConstantFolding.cpp

250

Isn't there still a bug here? The size of the result type of ptrtoint can disagree with its operand's type.

Sounds like the correct thing to do would be to call the static member function CastInst::isNoopCast with appropriate operands.

[arsenm]

lib/Analysis/ConstantFolding.cpp

250

This is what I thought at first to, but ptrtoints with noncanonical integer sizes seem to work. I'm not exactly sure why

[arsenm]

lib/Analysis/ConstantFolding.cpp

250

I think I understand why it isn't necessary to worry about that case. Because this doesn't look through inttoptr, we don't see pointer size changes through the ptrtoint/inttoptr path.

[hfinkel]

Accumulating the offset through the addrspacecast doesn't make much sense

Why does it not make sense? It just means that the base object is in a different address space from the original derived pointer. The LangRef says:

Note that if the address space conversion is legal then both result and operand refer to the same memory location.

and so, unless the addresscast were somehow illegal (is that possible for a constant expression addresscast?) then it should have a well-defined meaning. Am I missing something?

[arsenm]

In D11242#212156, @hfinkel wrote:

Accumulating the offset through the addrspacecast doesn't make much sense

Why does it not make sense? It just means that the base object is in a different address space from the original derived pointer. The LangRef says:

Note that if the address space conversion is legal then both result and operand refer to the same memory location.

and so, unless the addresscast were somehow illegal (is that possible for a constant expression addresscast?) then it should have a well-defined meaning. Am I missing something?

The addrspacecast can totally change the representation of the pointer. In the case I ran into, the original pointer is a 32-bit offset into special on chip memory being casted to a 64-bit address space. The pointer is converted to a full virtual 64-bit address which will resolve to the same location, but doesn't have a common base with the original pointer, so there isn't a simple add relationship between the two.

[hfinkel]

In D11242#212256, @arsenm wrote:

In D11242#212156, @hfinkel wrote:

Accumulating the offset through the addrspacecast doesn't make much sense

Why does it not make sense? It just means that the base object is in a different address space from the original derived pointer. The LangRef says:

Note that if the address space conversion is legal then both result and operand refer to the same memory location.

and so, unless the addresscast were somehow illegal (is that possible for a constant expression addresscast?) then it should have a well-defined meaning. Am I missing something?

The addrspacecast can totally change the representation of the pointer. In the case I ran into, the original pointer is a 32-bit offset into special on chip memory being casted to a 64-bit address space. The pointer is converted to a full virtual 64-bit address which will resolve to the same location, but doesn't have a common base with the original pointer, so there isn't a simple add relationship between the two.

Ah, okay. So the problem is then that you can't subtract them (or something like that). Or more specifically, the address space is part of the type, and constant folding can't change the type.

LGTM.

[arsenm]

r243300

lib/Analysis/ConstantFolding.cpp

if ((GV = dyn_cast<GlobalValue>(C))) {
     return true;
   }
 
   // Otherwise, if this isn't a constant expr, bail out.
   ConstantExpr *CE = dyn_cast<ConstantExpr>(C);
   if (!CE) return false;
 
   // Look through ptr->int and ptr->ptr casts.
   if (CE->getOpcode() == Instruction::PtrToInt ||
-      CE->getOpcode() == Instruction::BitCast ||
-      CE->getOpcode() == Instruction::AddrSpaceCast)
+      CE->getOpcode() == Instruction::BitCast)
     return IsConstantOffsetFromGlobal(CE->getOperand(0), GV, Offset, DL);
 
   // i32* getelementptr ([5 x i32]* @a, i32 0, i32 5)
   GEPOperator *GEP = dyn_cast<GEPOperator>(CE);
   if (!GEP)
     return false;
 
   unsigned BitWidth = DL.getPointerTypeSizeInBits(GEP->getType());

test/Transforms/ConstProp/loads.ll

 ; BE-LABEL: @test2(
 ; BE: ret i16 -8531
 }
 
 define i16 @test2_addrspacecast() {
   %r = load i16, i16 addrspace(1)* addrspacecast(i32* getelementptr ({{i32,i8},i32}, {{i32,i8},i32}* @g1, i32 0, i32 0, i32 0) to i16 addrspace(1)*)
   ret i16 %r
 
+; FIXME: Should be able to load through a constant addrspacecast.
 ; 0xBEEF
 ; LE-LABEL: @test2_addrspacecast(
-; LE: ret i16 -16657
+; XLE: ret i16 -16657
+; LE: load i16, i16 addrspace(1)* addrspacecast
 
 ; 0xDEAD
 ; BE-LABEL: @test2_addrspacecast(
-; BE: ret i16 -8531
+; XBE: ret i16 -8531
+; BE: load i16, i16 addrspace(1)* addrspacecast
 }
 
 ; Load of second 16 bits of 32-bit value.
 define i16 @test3() {
   %r = load i16, i16* getelementptr(i16, i16* bitcast(i32* getelementptr ({{i32,i8},i32}, {{i32,i8},i32}* @g1, i32 0, i32 0, i32 0) to i16*), i32 1)
   ret i16 %r
 
 ; 0xDEAD

test/Transforms/Inline/inline-constexpr-addrspacecast-argument.ll

+; RUN: opt -S -inline < %s | FileCheck %s
+
+target datalayout = "e-p3:32:32-p4:64:64-n32"
+
+@lds = internal addrspace(3) global [64 x i64] zeroinitializer
+
+; CHECK-LABEL: @constexpr_addrspacecast_ptr_size_change(
+; CHECK: load i64, i64 addrspace(4)* getelementptr (i64, i64 addrspace(4)* addrspacecast (i64 addrspace(3)* getelementptr inbounds ([64 x i64], [64 x i64] addrspace(3)* @lds, i32 0, i32 0) to i64 addrspace(4)*), i64 undef)
+; CHECK-NEXT: br
+define void @constexpr_addrspacecast_ptr_size_change() #0 {
+  %tmp0 = call i32 @foo(i64 addrspace(4)* addrspacecast (i64 addrspace(3)* getelementptr inbounds ([64 x i64], [64 x i64] addrspace(3)* @lds, i32 0, i32 0) to i64 addrspace(4)*)) #1
+  ret void
+}
+
+define i32 @foo(i64 addrspace(4)* %arg) #1 {
+bb:
+  %tmp = getelementptr i64, i64 addrspace(4)* %arg, i64 undef
+  %tmp1 = load i64, i64 addrspace(4)* %tmp
+  br i1 undef, label %bb2, label %bb3
+
+bb2:
+  store i64 0, i64 addrspace(4)* %tmp
+  br label %bb3
+
+bb3:
+  unreachable
+}
+
+attributes #0 = { nounwind }
+attributes #1 = { alwaysinline nounwind }