[PATCH] Fix SROA creating invalid bitcasts between address spaces

[Chandler Carruth]

  Matt, this change is simply not in a useful state to review, much less commit.

  To try to give you an idea of how broken this currently is, I spent the afternoon working through it.

  1) The change as posted doesn't pass its own tests. It appears you merged the test changes from D1765 into here, but not the code changes? I backed those out to get to a state that passed its own tests.
  2) Several of the changes which seem correct don't actually get exercised by any of your tests. It's not surprising as these changes are used to form GEPs and other complex pointers, and none of your test cases do anything like this. The only thing you have tested is doing memcpy across address spaces. So only those changes really make sense.
  3) One of the changes to the memcpy rewriting also didn't get exercised by your existing test cases. This is because that change is to the "vector-load" based memcpy rewriting rules and none of your test cases trigger vector rewriting.
  4) Right above the changes you made to memcpy rewriting is a totally separate (and important!) path for memcpy rewriting where we form a *new* memcpy. You didn't write any test cases for this. In fact, doing so would also provide a the root of a test case for the change Joey found (I don't know off hand what all would be required).
  5) Several of your changes are to code for rewriting loads and stores. The only loads and stores SROA currently rewrites are *alloca* loads and stores, so these changes are actually unnecessary given the current semantic constraint model which SROA operates under: alloca pointers are always in the default address space.
  6) Despite the changes I mention in #5, you didn't fix any of the *other* numerous places where we create loads, stores, or pointer casts in SROA for alloca-derived pointers, and so even if we were to allow allocas in different address spaces, the change as it stands doesn't seem like a principled change in that direction (although it might be a good incremental step).


  I now have a minimal patch and 3 test cases which fix half of memcpy rewriting. If you can provide a test case for vector-load/store based memcpy rewriting which exercises the 3'rd aspect of that change, I would be happy to submit that as a first step. That is what I would pursue as the next step to make forward progress.

  After that, you should provide test cases which exercise the other half of memcpy rewriting at all, and fixes the obvious bugs there.

  Finally you should provide test cases which form interesting pointer adjustments including complex "natural" GEPs in the process of memcpy rewriting in order to flush out the bugs inside of getAdjustedPtr (and getNaturalGEPWithOffset). This will probably also require the first change in D1765 (I think, this I haven't actually tested as I was trying to get through D1764).

  I have left comments in-line on the patch to highlight which changes are which, and to provide clarification.

  -Chandler


================
Comment at: lib/Transforms/Scalar/SROA.cpp:2111
@@ -2107,3 +2110,3 @@
     } else {
-      Type *LTy = TargetTy->getPointerTo();
+      Type *LTy = TargetTy->getPointerTo(AS);
       V = IRB.CreateAlignedLoad(
----------------
This change is unnecessary.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:2136
@@ -2132,3 +2135,3 @@
       Value *Placeholder
-        = new LoadInst(UndefValue::get(LI.getType()->getPointerTo()));
+        = new LoadInst(UndefValue::get(LI.getType()->getPointerTo(AS)));
       V = insertInteger(DL, IRB, Placeholder, V, NewBeginOffset,
----------------
This change is unnecessary.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:2247-2249
@@ -2243,4 +2246,5 @@
     } else {
+      unsigned AS = SI.getPointerAddressSpace();
       Value *NewPtr = getAdjustedAllocaPtr(IRB, NewBeginOffset,
-                                           V->getType()->getPointerTo());
+                                           V->getType()->getPointerTo(AS));
       NewSI = IRB.CreateAlignedStore(
----------------
This change is unnecessary.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:1318
@@ -1317,3 +1317,3 @@
   // an i8.
-  if (Ty == IRB.getInt8PtrTy() && TargetTy->isIntegerTy(8))
+  if (Ty == IRB.getInt8PtrTy(Ty->getAddressSpace()) && TargetTy->isIntegerTy(8))
     return 0;
----------------
No test case actually requires this change. I think this is actually necessary, but we really need a test case for it.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:1419-1420
@@ -1418,3 +1418,4 @@
     if (!Int8Ptr) {
-      Int8Ptr = IRB.CreateBitCast(Ptr, IRB.getInt8PtrTy(),
+      unsigned AS = Ptr->getType()->getPointerAddressSpace();
+      Int8Ptr = IRB.CreateBitCast(Ptr, IRB.getInt8PtrTy(AS),
                                   "raw_cast");
----------------
I also think this change is likely necessary, but again no test case fails if I delete it. We need a test case here as well.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:2541
@@ -2535,3 +2540,3 @@
 
-      OtherPtrTy = OtherPtrTy->getPointerTo();
+      OtherPtrTy = OtherPtrTy->getPointerTo(OtherAS);
     } else if (IntTy && !IsWholeAlloca) {
----------------
Undoing this change doesn't cause any test regressions. This change is *definitely* correct, but still needs a test case.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:1432
@@ -1431,3 +1432,3 @@
   if (Ptr->getType() != PointerTy)
     Ptr = IRB.CreateBitCast(Ptr, PointerTy, "cast");
 
----------------
Matt Arsenault wrote:
> Joey Gouly wrote:
> > Joey Gouly wrote:
> > > This line can also crash! I'll try get a test case.
> > Here is the test case, it runs with 'opt -sroa'.
> > 
> >   target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:64:128-a0:0:64-n32-S64"
> >   target triple = "armv7-none-linux-gnueabi"
> >   
> >   %struct.struct_test_27.0.13 = type { i32, float, i64, i8, [4 x i32] }
> >   
> >   ; Function Attrs: nounwind
> >   define void @copy_struct([5 x i64] %in.coerce) {
> >   for.end:
> >     %in = alloca %struct.struct_test_27.0.13, align 8
> >     %0 = bitcast %struct.struct_test_27.0.13* %in to [5 x i64]*
> >     store [5 x i64] %in.coerce, [5 x i64]* %0, align 8
> >     %scevgep9 = getelementptr %struct.struct_test_27.0.13* %in, i32 0, i32 4, i32 0
> >     %scevgep910 = bitcast i32* %scevgep9 to i8*
> >     call void @llvm.memcpy.p1i8.p0i8.i32(i8 addrspace(1)* undef, i8* %scevgep910, i32 16, i32 4, i1 false)
> >     ret void 
> >   }
> >   
> >   ; Function Attrs: nounwind 
> >   declare void @llvm.memcpy.p1i8.p0i8.i32(i8 addrspace(1)* nocapture, i8* nocapture readonly, i32, i32, i1) 
> > 
> This works for me. I have a couple more patches that fix SROA address spaces that I missed on the first pass, but they don't seem necessary to also fix this.
Joey, I have run this test case through opt with just the two diff hunks that are were already tested, and it passed for me every time I tried it.


http://llvm-reviews.chandlerc.com/D1764