[PATCH] D16382: Add LoopSimplifyCFG pass

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:37
@@ +36,3 @@
+namespace {
+  class LoopSimplifyCFG : public LoopPass {
+  public:
----------------
sanjoy wrote:
> 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.
Feel free to just throw some clang-format on this code since it is "new", it'll do the right thing for you. =]

================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:105
@@ +104,3 @@
+      L->moveToHeader(Succ);
+    LI->removeBlock(Pred);
+    MergeBasicBlockIntoOnlyPred(Succ, DT);
----------------
sanjoy wrote:
> 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`?
My suggestion would be to copy the loop blocks into a local smallvector, and then just loop over that so you don't have to worry about this and can freely update LoopInfo as you go.


Repository:
  rL LLVM

http://reviews.llvm.org/D16382