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

[Mehdi AMINI]

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


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:317
@@ -320,1 +316,3 @@
+static LoadInst *combineLoadToNewValue(InstCombiner &IC, LoadInst &LI, Value *V,
+                                       Type *NewTy, const Twine& Suffix = "") {
   SmallVector<std::pair<unsigned, MDNode *>, 8> MD;
----------------
Is there an invariant between V and NewTy here?
Should we add:

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

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:513
@@ -497,1 +512,3 @@
 
+static Instruction *unpackLoadToAggregate(InstCombiner &IC, LoadInst &LI) {
+  // FIXME: We could probably with some care handle both volatile and atomic
----------------
Missing documentation.

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

================
Comment at: test/Transforms/InstCombine/unpack-fca.ll:15
@@ -14,3 +14,3 @@
 
-define void @structA() {
+define void @storeA() {
 body:
----------------
When there are multiple tests in one file it is a good practice to use CHECK-LABEL for each function.

================
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
----------------
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.

http://reviews.llvm.org/D8339

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