Update InstCombine to transform aggregate loads into scalar loads.

[deadalnix]

Summary

One step further getting aggregate loads and store being optimized properly. This will only handle struct with one element at this point.

Depends on D7780

[deadalnix]

Update as per style
rebase

[deadalnix]

Better naming for unpacked loads.

[deadalnix]

Rebase and ping

[deadalnix]

It's been 3 weeks folk. Can we make something happen here, before we all die from old age ?

[majnemer]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

524

We know the type from the right hand side, please change this to auto *ST.

526

Is there any reason why you only go one level deep? What if we peel the struct type away just to find another single element struct type? Perhaps the InstCombine worklist will naturally get this... In any case, can you please have a test for this case?

test/Transforms/InstCombine/unpack-fca.ll

33

Please use CHECK-LABEL with the IR function's name for each test.

[mehdi_amini]

Can you provide a more complete description of what this is achieving in the commit message?

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

317

Is there an invariant between V and NewTy here?
Should we add:

assert((V->getType() == NewTy->getPointerTo(LI.getPointerAddressSpace())) && "V type mismatches NewTy");

512

Missing documentation.

522

Is this check useful? It seems redundant with the next one.
What about doing an early exit instead of adding nesting level:

StructType *ST = dyn_cast<StructType>(T);
// Only unpack structure with one element.
if(!ST || ST->getNumElements() != 1) return nullptr;

Ptr = IC.Builder->CreateStructGEP(Ptr, 0);
LoadInst *NewLoad = combineLoadToNewValue(IC, LI, Ptr, T, ".unpack");

Instruction *V = InsertValueInst::Create(UndefValue::get(T), NewLoad, 0, LI.getName());

LI.replaceAllUsesWith(V);
return V;

test/Transforms/InstCombine/unpack-fca.ll

15

When there are multiple tests in one file it is a good practice to use CHECK-LABEL for each function.

61

Use CHECK-NEXT to be sure that no load remains between the store and the ret (and makes explicit that you don't expect any other instruction here).
Alternatively you can also use

CHECK-NOT: load

after the check for the store.

[deadalnix]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

522

The code has it current structure because I intend to add support for arrays and multiple element structs. In this case, the early return do not make sense. The isAggregateType however, is indeed an early return in that case. I can update this if you feel strongly about it, but that would have to be undone is a subsequent diff.

526

Yes, InstCombine use a specific inserter in its builder. Also, this is what the structOfA test test for :)

test/Transforms/InstCombine/unpack-fca.ll

61

I never really intended to check this as this isn't really the goal of the diff, but I can add that.

[deadalnix]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

317

No, but they should be the same size. Adding this invariant.

[deadalnix]

Rebase and address various comments.

[mehdi_amini]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

522

OK, makes sense.

[reames]

Looks functionally correct to me. I do have some style comments that need addressed though.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

316

The naming here is confusing to the point where I'm having a hard time telling what you intend. If I'm reading this right, by Value you mean address loaded from? You're changing both the type loaded and the pointer operand in one go?

If so, I might suggest the following:

• pass both the pointer operand and new type explicitly. This will be necessary for the new unified pointer type work and helps readability now.
• Don't use the name V. Call it NewPointerOperand or something similiar. NewAddr would also be fine.
• Clarify the comment about what the function does.

You might also consider changing the type loaded and the pointer operand in two distinct steps.
NewLI = combineLoadToType(*this, OldLI, ElementType);
NewLI->setPointerOperand(ElementPtr);

The intermediate state (load type X from a pointer of type Y using a bitcast) is entirely legal.

745

I suspect your new code should be below these checks.

[deadalnix]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

316

setPointerOperand does not exists, but that is no big deal, a bitcast is just as valid here, so let's do that, it can make the diff much simpler.

[deadalnix]

Simplify the whole thing as suggested by @reames

[deadalnix]

Friday ping !

[pete]

You probably need to wait for some of the other commenters to give the final ok on this, but FWIW it LGTM with the changes I pointed out.

Cheers,
Pete

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

515

I think this should now be an assert as you've moved the call to unpackLoadToAggregate below an identical check.

519

This can just be LI.getType() instead of going through the pointer type.

521

If you're planning on handling arrays then please ignore me, but otherwise you don't need this check as you already explicitly check for struct types below.

[reames]

With Pete's comments addressed and the missing setPointerOperand(gep struct, 0) added, LGTM.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

530

You're missing the setPointerOperand step. Technically, the first element doesn't have to start at the beginning of the object. I think this is fine in practice, but there's a theoretical issue here.

[deadalnix]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

515

Other check also have this as a check rather than an invariant. I think this is ok as the applicability of these optimization will vary, and at the end we want the check there (considering optimizing non simple load/stores is making progress).

521

Yes, that is the plan. I had a complete solution, but I am breaking it up for easier review.

530

I'm not sure how this is possible at all.

[deadalnix]

Use LI.getType()

[reames]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

530

Unless the struct is packed, the data layout alignment could require that the internal field be aligned to a higher alignment than the struct. This would be a weird arrangement, but I believe it's possible.

[deadalnix]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

530

That would require the field to be at a variable position in the struct, which I don't think is supported.

Note that this piece of code is plugged AFTER the alignment is computed from the type for that very reason. Maybe I should add an assert to ensure alignment is set so it is won't break later.

[deadalnix]

[deadalnix]

rebase, add an assertion to ensure alignement is se is sett.

And let's get that in !

[deadalnix]

Rebase and ping.

[majnemer]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

316

Ampersand goes on the right hand side.

527–528

Did you intend to use i for any particular purpose? If not, please fold it into your call to getTypeAtIndex.

530–534

Can this be:

return ReplaceInstUsesWith(LI, Builder->CreateInsertValue(UndefValue::get(T), NewLoad, 0, LI.getName());

[deadalnix]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

527–528

Does not work. getTypeAtIndex has 2 overload, and passing 0 directly is ambiguous (in C++ 0 can also be a null pointer).

[deadalnix]

@majnemer comments

[majnemer]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

527–528

getTypeAtIndex(0U) should work.

[deadalnix]

Use 0U

[majnemer]

LGTM

[mehdi_amini]

Committed: r236695

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

if (AI.getAlignment()) {
     }
   }
 
   // At last, use the generic allocation site handler to aggressively remove
   // unused allocas.
   return visitAllocSite(AI);
 }
 
-/// \brief Helper to combine a load to a new type.
+/// \brief Helper to combine a load to a new value.
 ///
-/// This just does the work of combining a load to a new type. It handles
+/// This just does the work of combining a load to a new value. It handles
 /// metadata, etc., and returns the new instruction. The \c NewTy should be the
-/// loaded *value* type. This will convert it to a pointer, cast the operand to
-/// that pointer type, load it, etc.
+/// loaded *value* type.
 ///
 /// Note that this will create all of the instructions with whatever insert
 /// point the \c InstCombiner currently is using.
-static LoadInst *combineLoadToNewType(InstCombiner &IC, LoadInst &LI, Type *NewTy) {
-  Value *Ptr = LI.getPointerOperand();
-  unsigned AS = LI.getPointerAddressSpace();
+static LoadInst *combineLoadToNewValue(InstCombiner &IC, LoadInst &LI, Value *V, Type *NewTy) {
   SmallVector<std::pair<unsigned, MDNode *>, 8> MD;
   LI.getAllMetadata(MD);
 
   LoadInst *NewLoad = IC.Builder->CreateAlignedLoad(
-      IC.Builder->CreateBitCast(Ptr, NewTy->getPointerTo(AS)),
-      LI.getAlignment(), LI.getName());
+      V, LI.getAlignment(), LI.getName());
   MDBuilder MDB(NewLoad->getContext());
   for (const auto &MDPair : MD) {
     unsigned ID = MDPair.first;
     MDNode *N = MDPair.second;
     // Note, essentially every kind of metadata should be preserved here! This
     // routine is supposed to clone a load instruction changing *only its type*.
     // The only metadata it makes sense to drop is metadata which is invalidated
     // when the pointer type changes. This should essentially never be the case
case LLVMContext::MD_nonnull:
       // This only directly applies if the new type is also a pointer.
       if (NewTy->isPointerTy()) {
         NewLoad->setMetadata(ID, N);
         break;
       }
       // If it's integral now, translate it to !range metadata.
       if (NewTy->isIntegerTy()) {
         auto *ITy = cast<IntegerType>(NewTy);
+        Value *Ptr = LI.getPointerOperand();
         auto *NullInt = ConstantExpr::getPtrToInt(
             ConstantPointerNull::get(cast<PointerType>(Ptr->getType())), ITy);
         auto *NonNullInt =
             ConstantExpr::getAdd(NullInt, ConstantInt::get(ITy, 1));
         NewLoad->setMetadata(LLVMContext::MD_range,
                              MDB.createRange(NonNullInt, NullInt));
       }
       break;
 
     case LLVMContext::MD_range:
       // FIXME: It would be nice to propagate this in some way, but the type
       // conversions make it hard. If the new type is a pointer, we could
       // translate it to !nonnull metadata.
       break;
     }
   }
   return NewLoad;
 }
 
+/// \brief Helper to combine a load to a new type.
+///
+/// This just does the work of combining a load to a new type. It handles
+/// metadata, etc., and returns the new instruction. The \c NewTy should be the
+/// loaded *value* type. This will convert it to a pointer, cast the operand to
+/// that pointer type, load it, etc.
+///
+/// Note that this will create all of the instructions with whatever insert
+/// point the \c InstCombiner currently is using.
+static LoadInst *combineLoadToNewType(InstCombiner &IC, LoadInst &LI, Type *NewTy) {
+  Value *Ptr = LI.getPointerOperand();
+  unsigned AS = LI.getPointerAddressSpace();
+  
+  Value *V = IC.Builder->CreateBitCast(Ptr, NewTy->getPointerTo(AS));
+  return combineLoadToNewValue(IC, LI, V, NewTy);
+}
+
 /// \brief Combine a store to a new type.
 ///
 /// Returns the newly created store instruction.
 static StoreInst *combineStoreToNewValue(InstCombiner &IC, StoreInst &SI, Value *V) {
   Value *Ptr = SI.getPointerOperand();
   unsigned AS = SI.getPointerAddressSpace();
   SmallVector<std::pair<unsigned, MDNode *>, 8> MD;
   SI.getAllMetadata(MD);
if (auto *BC = dyn_cast<BitCastInst>(LI.user_back())) {
       return &LI;
     }
 
   // FIXME: We should also canonicalize loads of vectors when their elements are
   // cast to other types.
   return nullptr;
 }
 
+static Instruction *unpackLoadToAggregate(InstCombiner &IC, LoadInst &LI) {
+  // FIXME: We could probably with some care handle both volatile and atomic
+  // stores here but it isn't clear that this is important.
+  if (!LI.isSimple())
+    return nullptr;
+
+  Value *Ptr = LI.getPointerOperand();
+  Type *T = cast<PointerType>(Ptr->getType())->getElementType();
+
+  if (!T->isAggregateType())
+    return nullptr;
+
+  if (StructType *ST = dyn_cast<StructType>(T)) {
+    // If the struct only have one element, we unpack.
+    if (ST->getNumElements() == 1) {
+      Ptr = IC.Builder->CreateStructGEP(Ptr, 0);
+      LoadInst *NewLoad = combineLoadToNewValue(IC, LI, Ptr, T);
+
+      Instruction *V = InsertValueInst::Create(UndefValue::get(T), NewLoad, 0);
+
+      LI.replaceAllUsesWith(V);
+      return V;
+    }
+  }
+
+  return nullptr;
+}
+
 // If we can determine that all possible objects pointed to by the provided
 // pointer value are, not only dereferenceable, but also definitively less than
 // or equal to the provided maximum size, then return true. Otherwise, return
 // false (constant global values and allocas fall into this category).
 //
 // FIXME: This should probably live in ValueTracking (or similar).
 static bool isObjectSizeLessThanOrEq(Value *V, uint64_t MaxSize,
                                      const DataLayout &DL) {
Instruction *InstCombiner::visitLoadInst(LoadInst &LI) {
   unsigned EffectiveLoadAlign =
       LoadAlign != 0 ? LoadAlign : DL.getABITypeAlignment(LI.getType());
 
   if (KnownAlign > EffectiveLoadAlign)
     LI.setAlignment(KnownAlign);
   else if (LoadAlign == 0)
     LI.setAlignment(EffectiveLoadAlign);
 
+  if (Instruction *Res = unpackLoadToAggregate(*this, LI))
+    return Res;
+
   // Replace GEP indices if possible.
   if (Instruction *NewGEPI = replaceGEPIdxWithZero(*this, Op, LI)) {
       Worklist.Add(NewGEPI);
       return &LI;
   }
 
   // None of the following transforms are legal for volatile/atomic loads.
   // FIXME: Some of it is okay for atomic loads; needs refactoring.
   if (!LI.isSimple()) return nullptr;
 
   // Do really simple store-to-load forwarding and load CSE, to catch cases
   // where there are several consecutive memory accesses to the same location,
   // separated by a few arithmetic operations.
   BasicBlock::iterator BBI = &LI;
   if (Value *AvailableVal = FindAvailableLoadedValue(Op, LI.getParent(), BBI,6))
     return ReplaceInstUsesWith(
         LI, Builder->CreateBitOrPointerCast(AvailableVal, LI.getType(),

test/Transforms/InstCombine/unpack-fca.ll

 ; RUN: opt -instcombine -S < %s | FileCheck %s
 
 target datalayout = "e-i64:64-f80:128-n8:16:32:64"
 target triple = "x86_64-unknown-linux-gnu"
 
 %A__vtbl = type { i8*, i32 (%A*)* }
 %A = type { %A__vtbl* }
 
 @A__vtblZ = constant %A__vtbl { i8* null, i32 (%A*)* @A.foo }
 
 declare i32 @A.foo(%A* nocapture %this)
 
 declare i8* @allocmemory(i64)
 
-define void @structA() {
+define void @storeA() {
 body:
   %0 = tail call i8* @allocmemory(i64 32)
   %1 = bitcast i8* %0 to %A*
 ; CHECK: store %A__vtbl* @A__vtblZ
   store %A { %A__vtbl* @A__vtblZ }, %A* %1, align 8
   ret void
 }
 
-define void @structOfA() {
+define void @storeStructOfA() {
 body:
   %0 = tail call i8* @allocmemory(i64 32)
   %1 = bitcast i8* %0 to { %A }*
 ; CHECK: store %A__vtbl* @A__vtblZ
   store { %A } { %A { %A__vtbl* @A__vtblZ } }, { %A }* %1, align 8
   ret void
 }
+
+define %A @loadA() {
+body:
+  %0 = tail call i8* @allocmemory(i64 32)
+  %1 = bitcast i8* %0 to %A*
+; CHECK: load %A__vtbl*,
+; CHECK: insertvalue %A undef, %A__vtbl* {{.*}}, 0
+  %2 = load %A, %A* %1, align 8
+  ret %A %2
+}
+
+define { %A } @loadStructOfA() {
+body:
+  %0 = tail call i8* @allocmemory(i64 32)
+  %1 = bitcast i8* %0 to { %A }*
+; CHECK: load %A__vtbl*,
+; CHECK: insertvalue %A undef, %A__vtbl* {{.*}}, 0
+; CHECK: insertvalue { %A } undef, %A {{.*}}, 0
+  %2 = load { %A }, { %A }* %1, align 8
+  ret { %A } %2
+}
+
+define { %A } @structOfA() {
+body:
+  %0 = tail call i8* @allocmemory(i64 32)
+  %1 = bitcast i8* %0 to { %A }*
+; CHECK: store %A__vtbl* @A__vtblZ
+  store { %A } { %A { %A__vtbl* @A__vtblZ } }, { %A }* %1, align 8
+  %2 = load { %A }, { %A }* %1, align 8
+; CHECK: ret { %A } { %A { %A__vtbl* @A__vtblZ } }
+  ret { %A } %2
+}