SROA fix to avoid invalid bitcast generation when converting a load from a double pointer bitcasted with an address space change of the pointee pointee

[michele.scandale]

Summary

Here's a fix for SROA to avoid the generation of invalid bitcast between pointers in different address spaces.
The case is the following:

%p.addr = alloca <2 x float> addrspace(1)*, align 8
; ...
%p.new = bitcast <2 x float> addrspace(1)** %p.addr to <2 x float>**
%val = load <2 x float>** %p.new, align 8

SROA identifies an AllocaSlice of %p.addr and %val and tries to rewrite it removing the load. The problem is that the source type of the loaded value is <2 x float> addrspace(1)* and the destination type is <2 x float>*, thus a bitcast between those types is invalid.
The proposed fix catches these cases generating a ptrtoint+inttoptr pair instead of an addrspacecast to keep the common bits of the value immutated.

[arsenm]

My initial thought is you should not be introducing an ptrtoint if one doesn't already exist, and the problem is really the address space was not specified when necessary somewhere else.

[arsenm]

Actually I don't see the problem. I just tried your testcase with current trunk, and SROA makes no change to the IR. The testcase also involves bitcasts between pointers to pointers with an address space, which should be fine.

[michele.scandale]

I tested it at commit 122a970111b8ec66ae330f2c218ae951dddaf75b, and still I have an assertion on "castIsValid": the problem is exactly on pointer of pointer where the bitcast changes the address space of the pointee pointee.

define void @union_cvt_float2(<2 x float> addrspace(1)* %p) {
 %p.addr = alloca <2 x float> addrspace(1)*, align 8
 store <2 x float> addrspace(1)* %p, <2 x float> addrspace(1)** %p.addr, align 8
 %1 = bitcast <2 x float> addrspace(1)** %p.addr to <2 x float>**
 %2 = load <2 x float>** %1, align 8
 call void @ext_call_float2(<2 x float>* %2)
 ret void
}

In this example SROA wants to replace %2 = load <2 x float>** %1, align 8 with %2 = bitcast <2 x float> addrspace(1)* %p to <2 x float>*, but this is an invalid instruction!
The input IR does the following:

• bitcast the address of the stack location associated to the parameter (<2 x float> addrspace(1)*) to an address pointing to another type (<2 x float>*)
• load a <2 x float>* from the bitcasted address

The conservative assumption is that sizeof(<2 x float> addrspace(1)*) != <2 x float>*: in this case only the bits in the overlapped region are the same, thus IMHO the right transformation to preserve the semantic is to use ptrtoint+inttoptr using as middle type an integer type that can fully contains the original value.

[arsenm]

This same problem was reported here recently and this patch fixes it:

https://github.com/HSAFoundation/HLC-HSAIL-Development-LLVM/issues/17

The pointer sizes and formats are incompatible so there isn't really anything sensible to do other than to do this ptrtoint / inttoptr pair, so this makes sense to me.

lib/Transforms/Scalar/SROA.cpp

     // Expand i8* to <2 x i32> --> i8* to i64 to <2 x i32>
     if (!OldTy->isVectorTy() && NewTy->isVectorTy())
       return IRB.CreateBitCast(IRB.CreatePtrToInt(V, DL.getIntPtrType(OldTy)),
                                NewTy);
 
     return IRB.CreatePtrToInt(V, NewTy);
   }
 
+  // If both types are pointers in different address spaces use the pair
+  // ptrtoint+inttoptr to keep common bits of the value immutated.
+  if (OldTy->isPtrOrPtrVectorTy() && NewTy->isPtrOrPtrVectorTy() &&
+      OldTy->getPointerAddressSpace() != NewTy->getPointerAddressSpace())
+    return IRB.CreateIntToPtr(IRB.CreatePtrToInt(V, DL.getIntPtrType(OldTy)),
+                              NewTy);
+
   return IRB.CreateBitCast(V, NewTy);
 }
 
 /// \brief Test whether the given slice use can be promoted to a vector.
 ///
 /// This function is called to test each entry in a partioning which is slated
 /// for a single slice.
 static bool isVectorPromotionViableForSlice(

test/Transforms/SROA/union_ptr_as.ll

+; RUN: opt -S -sroa < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @union_cvt_float2(<2 x float> addrspace(1)* %p) {
+; CHECK: @union_cvt_float2
+; CHECK: %[[int:.*]] = ptrtoint <2 x float> addrspace(1)* %p to i64
+; CHECK-NEXT: %[[nptr:.*]] = inttoptr i64 %[[int]] to <2 x float>*
+
+  %p.addr = alloca <2 x float> addrspace(1)*, align 8
+  store <2 x float> addrspace(1)* %p, <2 x float> addrspace(1)** %p.addr, align 8
+  %1 = bitcast <2 x float> addrspace(1)** %p.addr to <2 x float>**
+  %2 = load <2 x float>** %1, align 8
+  call void @ext_call_float2(<2 x float>* %2)
+  ret void
+}
+
+define void @union_cvt_v2ptr(<2 x i8 addrspace(1)*> %p) {
+; CHECK: union_cvt_v2ptr
+; CHECK: %[[int:.*]] = ptrtoint <2 x i8 addrspace(1)*> %p to <2 x i64>
+; CHECK-NEXT: %[[nptr:.*]] = inttoptr <2 x i64> %[[int]] to <2 x i8*>
+
+  %p.addr = alloca <2 x i8 addrspace(1)*>, align 8
+  store <2 x i8 addrspace(1)*> %p, <2 x i8 addrspace(1)*>* %p.addr
+  %1 = bitcast <2 x i8 addrspace(1)*>* %p.addr to <2 x i8*>*
+  %2 = load <2 x i8*>* %1, align 8
+  call void @ext_call_v2ptr(<2 x i8*> %2)
+  ret void
+}
+
+declare void @ext_call_float2(<2 x float>*)
+declare void @ext_call_v2ptr(<2 x i8*>)