[PATCH] D55467: [WebAssembly] Optimize Irreducible Control Flow

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

First brief look at the pass.. mostly low-level nits

- Nit: Could you wrap comments to 80 cols? Some lines end prematurely now. (clang-format does not do that for us.. In vim you can use `gq`, but not sure about other editors)
- There are many lambda functions that can be made into plain static functions or class member functions. I think it is more conventional and readable that way unless they are very short or are meant to be passed into arguments to another function.. I feel it's not very easy to read when other several lambda function definitions are interleaved with the function body.



================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:85
     AU.addRequired<MachineDominatorTree>();
     AU.addPreserved<MachineDominatorTree>();
     AU.addRequired<MachineLoopInfo>();
----------------
I don't think we need dominator tree anymore, so I think we can delete these two lines.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:87
     AU.addRequired<MachineLoopInfo>();
     AU.addPreserved<MachineLoopInfo>();
     MachineFunctionPass::getAnalysisUsage(AU);
----------------
Do we preserve loop info?


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:109
 
-namespace {
-
-/// A utility for walking the blocks of a loop, handling a nested inner
-/// loop as a monolithic conceptual block.
-class MetaBlock {
-  MachineBasicBlock *Block;
-  SmallVector<MachineBasicBlock *, 2> Preds;
-  SmallVector<MachineBasicBlock *, 2> Succs;
-
-public:
-  explicit MetaBlock(MachineBasicBlock *MBB)
-      : Block(MBB), Preds(MBB->pred_begin(), MBB->pred_end()),
-        Succs(MBB->succ_begin(), MBB->succ_end()) {}
-
-  explicit MetaBlock(MachineLoop *Loop) : Block(Loop->getHeader()) {
-    Loop->getExitBlocks(Succs);
-    for (MachineBasicBlock *Pred : Block->predecessors())
-      if (!Loop->contains(Pred))
-        Preds.push_back(Pred);
-  }
-
-  MachineBasicBlock *getBlock() const { return Block; }
+bool WebAssemblyFixIrreducibleControlFlow::VisitLoop(MachineFunction &MF,
+                                                     MachineLoopInfo &MLI,
----------------
Nit: [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | LLVM coding standards ]] say function names should start with a lowercase letter.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:159
+  // Compute which (canonicalized) blocks each block can reach.
+  typedef std::unordered_set<MachineBasicBlock *> BlockSet;
+  std::unordered_map<MachineBasicBlock *, BlockSet> Reachable;
----------------
Nit: `using` instead of `typedef`


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:160
+  typedef std::unordered_set<MachineBasicBlock *> BlockSet;
+  std::unordered_map<MachineBasicBlock *, BlockSet> Reachable;
+
----------------
[[ https://llvm.org/docs/CodingStandards.html#beware-of-non-determinism-due-to-ordering-of-pointers | LLVM coding standards ]] warn against uses of non-deterministic containers. Also LLVM progamming manual prevents the uses of `unordered_set` or `unordered_map`. ([[ http://llvm.org/docs/ProgrammersManual.html#other-set-like-container-options | ref1 ]], [[ http://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options | ref2 ]])

Maybe we can choose from other [[ http://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc | set-like containers ]] and [[ http://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options | map-like containers ]]?


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:164
+  // added a link a => b.
+  typedef std::pair<MachineBasicBlock *, MachineBasicBlock *> BlockPair;
+  std::vector<BlockPair> WorkList;
----------------
Nit: `using` instead of `typedef`


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:179
+      if (CanonicalizeSuccessor(MBB)) {
+        WorkList.push_back(BlockPair(MBB, Succ));
+      }
----------------
Nit: You can do
```
WorkList.emplace_back(MBB, Succ);
```


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:214
+    auto *MBB = Pair.first;
+    auto *Succ = Pair.second;
+    assert(MBB);
----------------
Nit: You can merge these lines maybe with
```
auto *MBB, *Succ;
std::tie(MBB, Succ) = WorkList.pop_back_val()
```


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:382
+        MF.RenumberBlocks();
+        getAnalysis<MachineDominatorTree>().runOnMachineFunction(MF);
+        MLI.runOnMachineFunction(MF);
----------------
Should we recompute these three every time we make a change?
- This pass does not use liveness info, so I think we can do this at the end of the whole pass if any change was made.
- I don't think we need to do renumbering? We only use BB numbers at CFGSort / CFGStackify and CFGSort recomputes all numbers itself before doing anything.
- This pass does not seem to use dominator tree, right? Then deleting `AU.addPreserved<MachineDominatorTree>();` in `getAnalysisUsage()` should be sufficient. I don't think we even need to require it.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:383
+        getAnalysis<MachineDominatorTree>().runOnMachineFunction(MF);
+        MLI.runOnMachineFunction(MF);
+        return Changed = true;
----------------
Should we recompute loop info for the whole function every time we make any change? `MachineLoopInfo` class seems to have [[ https://github.com/llvm-mirror/llvm/blob/2ca3d46c8a27e6de8a99de6bdc08e0884e8e6a85/include/llvm/CodeGen/MachineLoopInfo.h#L142-L169 | various modifier functions ]]. Can we use these on-the-fly during the pass?


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:405
 
-  // If we made any changes, completely recompute everything.
-  if (LLVM_UNLIKELY(Changed)) {
-    LLVM_DEBUG(dbgs() << "Recomputing dominators and loops.\n");
-    MF.getRegInfo().invalidateLiveness();
-    MF.RenumberBlocks();
-    getAnalysis<MachineDominatorTree>().runOnMachineFunction(MF);
-    MLI.runOnMachineFunction(MF);
+  while (Iteration()) {
   }
----------------
How about flattening `Iteration` and `DoVisitLoop` into this while loop, like this? I find nested lambdas a bit hard to read.

```                   
  while (...) {                                                                 
    SmallVector<MachineLoop *, 8> Worklist;                                                    
    // Visit the function body, which is identified as a null loop.              
    Worklist.push_back(nullptr);                                                 
    // Visit all the loops.                                                      
    Worklist.append(MLI.begin(), MLI.end());      
                               
    while (!Worklist.empty()) {                                                  
      MachineLoop *Loop = Worklist.pop_back_val();                               
      Worklist.append(Loop->begin(), Loop->end());                                                          
      if (VisitLoop(MF, MLI, Loop)) {           
        ...                                 
      }                                                                     
    }                                                                            
  }
```


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

https://reviews.llvm.org/D55467