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

[Johannes Doerfert via Phabricator via llvm-commits]

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

I added various formatting and code comments but other than that, this LGTM.



================
Comment at: llvm/include/llvm/Transforms/Utils/ImproveReadingOrder.h:18
+
+using namespace llvm;
+
----------------
Don't open a namespace in a header.

```
namespace llvm {
...
}
```


================
Comment at: llvm/include/llvm/Transforms/Utils/ImproveReadingOrder.h:37
+struct ImproveReadingOrder : public PassInfoMixin<ImproveReadingOrder> {
+  ImproveReadingOrder() = default;
+  PreservedAnalyses run(Function &F, FunctionAnalysisManager &FAM);
----------------
Most "new PM" transformations only expose the run method. See the `preferredNextBlock` definition comment.


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:15
+
+#include <set>
+
----------------
Leftover


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:57
+  SmallPtrSet<BasicBlock *, 16> BlocksWithUnreachables;
+  for (auto &b : F.getBasicBlockList())
+    if (isa<UnreachableInst>(b.getTerminator()))
----------------
Variable names commonly start with an upper case letter: `b` -> `BB`.
You can also simply "iterate" over F: `BasicBlock &BB : F`


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:96
+             Terminator->getNumSuccessors() + 1);
+      std::map<std::pair<uint, uint>, BasicBlock *> Ordered;
+      uint n = 0;
----------------
`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);`


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:99
+      while (n < Terminator->getNumSuccessors()) {
+        ConstantInt *weight =
+            mdconst::dyn_extract<ConstantInt>(BranchWeights->getOperand(n + 1));
----------------
`Weight`


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:109
+      for (auto it : Ordered)
+        Successors.push_back(it.second);
+    } else {
----------------
With a vector, as described above, it will be a `Successors.append(Ordered.begin(), Ordered.end())`.


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:121
+    BasicBlock *HeadingTowardsUnreachable = nullptr;
+    for (auto u : BlocksWithUnreachables)
+      if (!HeadingTowardsUnreachable && PDT.dominates(u, Current))
----------------
`auto u` -> `BasicBlock *BB`


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:123
+      if (!HeadingTowardsUnreachable && PDT.dominates(u, Current))
+        HeadingTowardsUnreachable = u;
+    for (auto s : Successors) {
----------------
It took me a while to see that you only want to find one witness:
Add a break, remove the `!HadingTowardsUnreachable`. 


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:131
+        if (LeadsToUnreachable) {
+          ExceptionHandlers.insert(s);
+        } else {
----------------
I don't understand the interplay between `HeadingTowardsUnreachable` and `LeadsToUnreachable`. Maybe add a comment to explain.


================
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);
----------------
I would assume you could use LoopInfo and the nesting depth of loops to reason here as well.


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:156
+ImproveReadingOrder::preferredNextBlock(SetVector<BasicBlock *> &Input,
+                                        const PostDominatorTree &PDT) {
+  BasicBlock *Result = nullptr;
----------------
If you move this at the beginning of the file you can make it static.


================
Comment at: llvm/lib/Transforms/Utils/ImproveReadingOrder.cpp:165
+    PDT.getDescendants(Result, PossiblePredecessors);
+    for (auto i : PossiblePredecessors) {
+      if (i != Result && Input.count(i)) {
----------------
`i` is associated with integers (in my mind).` BasicBlock &PossiblePredBB` maybe


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60619