[PATCH] Update InstCombine to transform aggregate loads into scalar loads.

[Amaury SECHET]

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:523
@@ +522,3 @@
+  if (!T->isAggregateType())
+    return nullptr;
+
----------------
joker.eph wrote:
> 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;
> ```
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.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:527
@@ +526,3 @@
+    // If the struct only have one element, we unpack.
+    if (ST->getNumElements() == 1) {
+      Ptr = IC.Builder->CreateStructGEP(Ptr, 0);
----------------
majnemer wrote:
> 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?
Yes, InstCombine use a specific inserter in its builder. Also, this is what the structOfA test test for :)

================
Comment at: test/Transforms/InstCombine/unpack-fca.ll:61
@@ +60,3 @@
+  %2 = load { %A }, { %A }* %1, align 8
+; CHECK: ret { %A } { %A { %A__vtbl* @A__vtblZ } }
+  ret { %A } %2
----------------
joker.eph wrote:
> 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.
I never really intended to check this as this isn't really the goal of the diff, but I can add that.

http://reviews.llvm.org/D8339

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/