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

[Philip Reames]

I'm okay with what's here once my fairly minor comments are addressed, but let's wait for Chandler to take a look as well.

(And thank you again for sticking it through a long review process!)


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:811
@@ +810,3 @@
+static bool unpackStoreToAggregate(InstCombiner &IC, StoreInst &SI) {
+  // FIXME: We could probably with some care handle both volatile and atomic
+  // stores here but it isn't clear that this is important.
----------------
Rather than returning bool, please return the new Instruction or nullptr.  This will allow us to add that new instruction to the worklist and will solve the deeply nested struct thing you were trying to address with recursion in the previous patch.  

Alternatively, you could simple add the newly created store to the worklist within this function.  I have no strong preference, but I suspect Chandler may care.  It might be good to let him comment before actually making either change.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:816
@@ +815,3 @@
+
+  Value* V = SI.getValueOperand();
+  Type* T = V->getType();
----------------
nit: Value *V

You should probably run this through clang-format.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:825
@@ +824,3 @@
+    if (ST->getNumElements() == 1) {
+      V = IC.Builder->CreateExtractValue(V, 0);
+      combineStoreToNewValue(IC, SI, V);
----------------
Add a comment here that highlights the fact that the wrapped value can be smaller than the FCA and that we are possibly loosing dereferencability information here.  We're okay with that for now, but it would be good to have it documented.

================
Comment at: test/Transforms/InstCombine/aggregate-store.ll:15
@@ +14,3 @@
+
+define void @structs() {
+body:
----------------
Once you get the new stores onto the worklist, please add a test case for the nested struct case.

http://reviews.llvm.org/D7780

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