[PATCH] D75865: Introduce unify-loop-exits pass.

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle accepted this revision.
nhaehnle added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM!



================
Comment at: llvm/lib/Transforms/Utils/UnifyLoopExits.cpp:139-141
+  // Locate exits by examining the successors of the exiting blocks. This is
+  // cheaper than calling Loop::getExitBlocks(), since that goes through the
+  // entire list of blocks in the loop.
----------------
sameerds wrote:
> nhaehnle wrote:
> > I don't understand this argument. getExitingBlocks also goes through all blocks in the loop... since you need both exiting *and* exit blocks, you may as well loop over all blocks in the loop directly. Though I guess this approach is not too bad, either, just the comment is confusing. If you want to argue that getting the exiting blocks and then enumerating successors to find exits is cheaper than enumerating exits and then enumerating predecessors, I suppose there are two arguments to be made:
> > * Eliminating duplicates is more natural the way you're doing it
> > * Iterating over successors is more efficient than iterating over predecessors in LLVM IR
> I assume you are okay with the call to getExitingBlocks. If I were to traverse the whole loop body myself, that would just duplicate functionality that's already easily available. I rewrote the comments for clarity.
Yes.


================
Comment at: llvm/lib/Transforms/Utils/UnifyLoopExits.cpp:196
+  }
+  LI.verify(DT);
+
----------------
sameerds wrote:
> nhaehnle wrote:
> > This should be guarded by `#ifndef NDEBUG`.
> Wrapped this in EXPENSIVE_CHECKS instead.
Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75865/new/

https://reviews.llvm.org/D75865