[PATCH] D60619: New pass to produce more easily-read IR.

[Arnt Gulbrandsen via Phabricator via llvm-commits]

arnt marked 9 inline comments as done.
arnt added a comment.

Updated diff coming in a few minutes...



================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:52
+  BasicBlock * lastSoFar = nullptr;
+  std::set<BasicBlock *> processed;
+  std::vector<BasicBlock *> toBeDone;
----------------
MaskRay wrote:
> For a not-necessarily-sorted set, you may use `SmallPtrSet` or `DenseSet`.
I kept SetVector; the performance isn't a problem and its order produces nice results. (Performance is generally not a problem here since the pass's application limits the data size. A million-line function isn't readable in any reading order, see?)


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:96
+             Terminator->getNumSuccessors() + 1);
+      std::map<std::pair<uint, uint>, BasicBlock *> Ordered;
+      uint n = 0;
----------------
fhahn wrote:
> jdoerfert wrote:
> > `uint` -> `uint64_t` (to match `getZExtValue`)
> > Also, why do you do sorting through a `std::map`? Is there a guarantee that it will be sorted (in the way you need it)?
> > I'd put all in a `SmallVector<std::pair<uint64_t, uint64_t>, 8> Ordered.`, then
> > call `llvm::sort(Ordered);`
> Or use MapVector, if it accepts std::pair as KeyT https://www.llvm.org/docs/ProgrammersManual.html#llvm-adt-mapvector-h
There must be dozens of ways to get the order I want (heaviest branch weight first, falling back to input order), all of which have essentially the same performance: fast enough for any function a human can aspire to read.

Why did I pick the one I picked? Most likely because I had worked on STL-heavy code in the days before, and used the same idioms. IMO it doesn't matter. MapVector would be as good, but changing now would just be effort without significant improvement.


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:131
+        if (LeadsToUnreachable) {
+          ExceptionHandlers.insert(s);
+        } else {
----------------
jdoerfert wrote:
> I don't understand the interplay between `HeadingTowardsUnreachable` and `LeadsToUnreachable`. Maybe add a comment to explain.
OK... but let me try here first. It's about happypaths, sad paths and even sadder paths, like code that throws an exception while trying to throw or handle another exception.

Suppose that a function contains a couple of blocks that end with a return and a couple of blocks that end with unreachables. In that case the pass will generally try to output everything that can lead to a return first, but eventually it has to start outputting the blocks whose successors end with either/both unreachables.

Once it has started on  a block that is postdominated by one of those unreachables, then it's best to output everything related to that unreachable. For example, if the two unreachables are for throwing "null pointer dereferenced" and "index out of bounds", then it's best to print all of the null-pointer blocks in one group, and all of the index-checking blocks in another group.

So that's what it tries to do: If the current block is already heading for an unreachable, then distinguishes between blocks that lead to the "current" unreachable, and other, even sadder paths unreachables.

I renamed one variable and added a comment. Thanks. Compiling now.


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:134
+          // later insertion has to win over earlier so that inner loops are
+          // inside outer loops
+          ToBeDone.remove(s);
----------------
jdoerfert wrote:
> I would assume you could use LoopInfo and the nesting depth of loops to reason here as well.
I would assume so too, but I still haven't come across any functions where I'm unsatisified with the output order and think that LoopInfo would help.

(I've used this to read code for most of a year now.)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60619