[PATCH] D16382: Add LoopSimplifyCFG pass

[escha via llvm-commits]

escha added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:37
@@ +36,3 @@
+namespace {
+  class LoopSimplifyCFG : public LoopPass {
+  public:
----------------
sanjoy wrote:
> Minor: convention is to not indent namespaces: http://llvm.org/docs/CodingStandards.html#namespace-indentation
> 
> Also, why not make this a struct?
I was just copying LoopDeletion to make this pass; has the style changed since then?

================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:90
@@ +89,3 @@
+
+  bool changed = false;
+  DominatorTree *DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
----------------
sanjoy wrote:
> Minor: coding style is to name this `Changed`.
Okay, will change this on the next update.

================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:105
@@ +104,3 @@
+      L->moveToHeader(Succ);
+    LI->removeBlock(Pred);
+    MergeBasicBlockIntoOnlyPred(Succ, DT);
----------------
sanjoy wrote:
> Won't this invalidate `E`?
I don't *think* so? This only modifies things earlier in the list, not later. Should I recalculate E on each iteration just to be sure?

================
Comment at: test/Transforms/LoopSimplifyCFG/merge-header.ll:4-5
@@ +3,4 @@
+; CHECK-LABEL: foo
+; CHECK: entry
+; CHECK-NEXT: br label %inner
+define i32 @foo(i32* %P, i64* %Q) {
----------------
mzolotukhin wrote:
> Which basic blocks do we merge here? From the check-lines it looks like we merge `%entry` with `%outer`, which doesn't sound right, since we want to merge only blocks inside loops. Am I misreading something here?
We want to merge Entry and Outer here, yes. Entry is the loop header, Outer + Inner are the body, Outer.Latch2 is the latch, and Exit is the loop exit. Entry + Outer can be merged in this case.


Repository:
  rL LLVM

http://reviews.llvm.org/D16382