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

[Philip Reames]

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


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:316
@@ -316,4 +315,3 @@
 /// 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, const Twine& Suffix = "") {
----------------
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.  



================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:756
@@ -701,3 +755,3 @@
   // FIXME: Some of it is okay for atomic loads; needs refactoring.
   if (!LI.isSimple()) return nullptr;
 
----------------
I suspect your new code should be below these checks.

http://reviews.llvm.org/D8339

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