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

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

Hi Amaury,

Overall that seems pretty good, I'm glad you find a way to leverage SROA.

I feel the implementation could benefit from more comments/description. You'll find some comments inline.

Best,

Mehdi


================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:212
@@ +211,3 @@
+  // Lift aggregate load/store into alloca
+  MPM.add(createAggregateLifterPass());
+
----------------
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?

================
Comment at: lib/Transforms/Scalar/AggregateLifter.cpp:10
@@ +9,3 @@
+/// \file
+/// This transforms removes load ans stores of aggregate type by replacing them
+/// by a memcpy into an alloc and rewrting IR to use it. This will allow
----------------
s/ans/and/

================
Comment at: lib/Transforms/Scalar/AggregateLifter.cpp:11
@@ +10,3 @@
+/// This transforms removes load ans stores of aggregate type by replacing them
+/// by a memcpy into an alloc and rewrting IR to use it. This will allow
+/// subsequent passes, notably SROA, to optimize properly.
----------------
s/rewrting/rewriting/

================
Comment at: lib/Transforms/Scalar/AggregateLifter.cpp:77
@@ +76,3 @@
+    FixupMap CurrentWorklist;
+    std::swap(CurrentWorklist, FixupWorklist);
+
----------------
It seems this is needed because `FixupWorklist` is a map. But it is not clear to me why are you needing the map behavior?

================
Comment at: lib/Transforms/Scalar/AggregateLifter.cpp:91
@@ +90,3 @@
+    InstrsToErase[i]->eraseFromParent();
+  }
+
----------------
for-range?

================
Comment at: lib/Transforms/Scalar/AggregateLifter.cpp:111
@@ +110,3 @@
+
+  unsigned Size = DL.getTypeStoreSize(T); // getTypeAllocSize ?
+  unsigned Align = LI->getAlignment();
----------------
`getTypeStoreSize()` is fine for the `memcpy` I think.
But since you get it just before creating the alloca, the name `Size` for the variable can be ambiguous. What about either moving it closer to its use, or renaming it something like `StoreSize`. 

================
Comment at: lib/Transforms/Scalar/AggregateLifter.cpp:132
@@ +131,3 @@
+  Function *F = SI->getParent()->getParent();
+  const DataLayout &DL = F->getParent()->getDataLayout();
+
----------------
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.


================
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();
----------------
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)


http://reviews.llvm.org/D12269