[PATCH] D16382: Add LoopSimplifyCFG pass

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:37
@@ +36,3 @@
+namespace {
+  class LoopSimplifyCFG : public LoopPass {
+  public:
----------------
escha wrote:
> 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?
My guess would be that that indentation rule was added after `LoopDeletion` was added to LLVM.  In any case, it is in the coding standard now, so we should just follow that. :)

If you have spare time, I think fixing the indentation in LoopDeletion in a separate change will also be a appropriate thing to do.

================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:105
@@ +104,3 @@
+      L->moveToHeader(Succ);
+    LI->removeBlock(Pred);
+    MergeBasicBlockIntoOnlyPred(Succ, DT);
----------------
majnemer wrote:
> escha wrote:
> > 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?
> `Loop::block_end` accesses the end iterator of a vector. `LoopInfo::removeBlock` calls `Loop::removeBlockFromLoop` which will `erase` an element from the vector.  `std::vector::erase` invalidates all iterators at or beyond the point of the erase including the vector's `end` iterator.
Now that I think of it, is the `++I` safe?  Won't `I` be invalidated by the `erase`?


Repository:
  rL LLVM

http://reviews.llvm.org/D16382