[PATCH] D12269: Add a pass to lift aggregate into allocas so SROA can get rid of them.

[Amaury SECHET via llvm-commits]

deadalnix added inline comments.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:212
@@ +211,3 @@
+  // Lift aggregate load/store into alloca
+  MPM.add(createAggregateLifterPass());
+
----------------
joker.eph wrote:
> I think this should probably go just before SROA, either at the three places it is inserted in this file, or only before the one in the FPM. I tend to think the latter is more appropriate, unless there is room for the inliner to expose more cases than can be handled.
> 
> 
> What is the impact of this transformation on the code generated by clang? Should it be enabled by a flag only?
Yeah i inserted it pretty randomly. clang should not be affected by this as it doesn't generate aggregate loads/stores.

================
Comment at: lib/Transforms/Scalar/AggregateLifter.cpp:77
@@ +76,3 @@
+    FixupMap CurrentWorklist;
+    std::swap(CurrentWorklist, FixupWorklist);
+
----------------
joker.eph wrote:
> It seems this is needed because `FixupWorklist` is a map. But it is not clear to me why are you needing the map behavior?
In this diff, I don't really need. But you onlyhave the "top-down" part of the problem here : you get a load and lift from there.

In the case you get a store of a value that not come from a load to begin with, you need to go bottom up ie you lift starting from the store. While going bottom-up, you need to check if some plan exists for other aggregate you encounter, and this is where the map semantic is needed.

================
Comment at: lib/Transforms/Scalar/AggregateLifter.cpp:91
@@ +90,3 @@
+    InstrsToErase[i]->eraseFromParent();
+  }
+
----------------
joker.eph wrote:
> for-range?
Is there a simple way to go backward using range ? construct à la range(rbegin rend) are not very handy.

================
Comment at: lib/Transforms/Scalar/AggregateLifter.cpp:132
@@ +131,3 @@
+  Function *F = SI->getParent()->getParent();
+  const DataLayout &DL = F->getParent()->getDataLayout();
+
----------------
joker.eph wrote:
> What about refactoring with an `AggregateLifterPass` that creates a transient object `AggregateLifter` which is not a pass, takes a reference to a Function and keep it as a member, and initialize a DataLayout reference member. (and make `liftStore` a private member)
> This pattern avoid having to dereference multiple pointer all over the place to get back to the parent Function and the DataLayout. Also having less state in the Pass itself seems cleaner to me in general. I see the pass as a wrapper for the transformation only (single responsibility). I also believe it makes it easier to reuse, for instance in the new PassManager interface.
> 
Sounds good.

================
Comment at: lib/Transforms/Scalar/AggregateLifter.cpp:136
@@ +135,3 @@
+  unsigned Size = DL.getTypeStoreSize(T); // getTypeAllocSize ?
+  // What if this is smaller than alloca's align ?
+  unsigned Align = SI->getAlignment();
----------------
joker.eph wrote:
> Smaller is not a correctness issue right? I think larger might be more problematic.
> 
> (Also at this point you might be storing somewhere else than at the beginning of the alloca, processing a GEP)
Yeah the comment is wrong.<

Also, I'm not sure what if, that is exactly why this comment is here. Thinking more about it, I think there may be a correctness concern here, I have to think more about this.


http://reviews.llvm.org/D12269