[PATCH] D58919: WebAssembly: Irreducible control flow rewrite

Tue Mar 12 05:33:05 PDT 2019

aheejin added a comment.

Sorry for the delayed reply!

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:30
+///   O(NumBlocks * NumNestedLoops * NumIrreducibleLoops +
+///     NumLoops * NumLoops)
 ///
----------------
Why is this?

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:144
+      MachineBasicBlock *MBB;
+      MachineBasicBlock *Succ;
+      std::tie(MBB, Succ) = WorkList.pop_back_val();
----------------
Real nit: We can merge these into a single line:
```
MachineBasicBlock *MBB, *Succ;
```

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:158

-  // Potentially insert a new reachable edge, and if so, note it as further
-  // work.
-  void maybeInsert(MachineBasicBlock *MBB, MachineBasicBlock *Succ) {
-    assert(MBB == canonicalize(MBB));
-    assert(Succ);
-    // Succ may not be interesting as a sucessor.
-    Succ = canonicalizeSuccessor(Succ);
-    if (!Succ)
-      return;
-    if (Reachable[MBB].insert(Succ).second) {
-      // For there to be further work, it means that we have
-      //   X => MBB => Succ
-      // for some other X, and in that case X => Succ would be a new edge for
-      // us to discover later. However, if we don't care about MBB as a
-      // successor, then we don't care about that anyhow.
-      if (canonicalizeSuccessor(MBB)) {
-        WorkList.emplace_back(MBB, Succ);
+    // Blocks that can return to themselves are in a loop.
+    for (auto *MBB : Blocks) {
----------------
We used to have the description on the definitions of 'loopers' and 'non-loopers' in the comment at the beginning of the file, but not anymore. Can we have that again here maybe?

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:167
+    // Find the loop entries - loopers reachable from non-loopers - and their
+    // loop enterers.
+    for (auto *Looper : Loopers) {
----------------
Does this definition cover when `Entry` is an entry (or header) of a loop? I'm wondering because the reachability computation here does not consider backedges to `Entry`, so it will not be discovered as a looper.

Also does it cover this case?
```
bb0:
  br bb1
bb1:
  br_if bb0
  br bb2
bb2:
  br bb3
bb3:
  br bb2
```

So here bb0 and bb1 comprise a loop and bb2 and bb3 comprise another loop. And all four blocks are loopers, but bb0 and bb2 are loop entries. But they are not reachable from any of non-loopers, because there are no non-loopers in this CFG.
(Also here bb0 is the header of a loop so it belongs to the first case where `Entry` is a header)

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:201
+    // from outside, we will traverse all the blocks in the loop.
+    BlockSet WorkList;
+    Blocks.insert(Entry);
----------------
Why is this worklist a set?

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:222
+
+class LoopFixer {
+public:
----------------
Do we need to make this as a separate class now? Unlike the previous version, we create `LoopFixer` object only once in `WebAssemblyFixIrreducibleControlFlow::runOnMachineFunction`. I think we can add paste the body of `LoopFixer::run` into `WebAssemblyFixIrreducibleControlFlow::runOnMachineFunction` and make `LoopFixer:: processRegion` as a member function of `WebAssemblyFixIrreducibleControlFlow` that returns a bool.

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:270
+        }
       }
+      // Only go on to actually process the inner loops when we are done
----------------
I might be mistaken, but the algorithm seems to not care about whether loop entries found are within the same level loop. For example, the case where there are two-level nested loops and block A is one of the entries to the outer loop and B is one of the entries for the inner loop. Even in this case, we don't distinguish them and resolve these with a single pass with a single dispatch block. Is this OK and intended? And for the 'mutual entries' above, can't they also be not in the same loop but each in an outer loop and an inner loop?

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:275
         continue;
       }

----------------
- How about reducing depth of the subloop processing routine below by wrapping this loop here early, like
```
while (FoundIrreducibility) {
  ReachabilityGraph Graph(Entry, Blocks);

  bool FoundIrreducibility = false;
  for (auto *LoopEntry : Graph.getLoopEntries()) {
    ...
  }
}

for (auto *LoopEntry : Graph.getLoopEntries()) {
  ...
}
```

- Btw, is there any case we can find irreducibility again with the same region //before// we recurse into inner loops below? If there is, how is that case different from the case that's processed below on the inner loops?

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:286
+        processRegion(LoopEntry, InnerBlocks.getBlocks());
       }

----------------
In which case do we need to recurse into inner loops? (Given that we didn't distinguish inner loops and outer loops above) Could you add some comments?

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:303
+      SortedEntries.push_back(Entry);
+    }
+    llvm::sort(SortedEntries,
----------------
Nit: We can do in a single line
```
BlockVector SortedEntries(Entries.begin(), Entries.end());
```

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:347
+        continue;
+      }

----------------
Is there any case `!Pair.second` is not set? Aren't all blocks in `SortedEntries` distinct?

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:371
+      DenseMap<MachineBasicBlock *, MachineBasicBlock *> Map;
+      for (auto *Entry : Pred->successors()) {
+        if (!Entries.count(Entry)) {
----------------
This loop does not seem to check if a corresponding `Split` block for `Entry` has been already created. In case `Entry` has more than one predecessors, `Split` for `Entry` is gonna be created multiple times and `Map[Entry]` will be overwritten. We should do something like this inside the loop:
```
if (Map.count(Entry))
  continue;
```

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:378
+        MachineBasicBlock *Split = MF.CreateMachineBasicBlock();
+        MF.insert(Pred->isLayoutSuccessor(Entry)
+                      ? MachineFunction::iterator(Entry)
----------------
Did you mean
```
Entry->isLayoutSuccessor(Pred)
```
? In the current state all new blocks are gonna be appended at the end, because this condition is hardly likely to be satisfied.

================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:390
+        BuildMI(*Split, Split->end(), DebugLoc(), TII.get(WebAssembly::BR))
+            .addMBB(Dispatch);
+        Split->addSuccessor(Dispatch);
----------------
Nit: There is a [[ https://github.com/llvm/llvm-project/blob/a946997c2482e4386549ee38b4bb154eb58efbb6/llvm/include/llvm/CodeGen/MachineInstrBuilder.h#L405-L410 | `BuildMI` function ]] that defaults to append instruction at the end, so you can just do
```
BuildMI(Split, DebugLoc(), TII.get(WebAssembly::CONST_I32), Reg)
    .addImm(Indices[Entry]);                                              
BuildMI(Split, DebugLoc(), TII.get(WebAssembly::BR)).addMBB(Dispatch);
```
(Note that `Split` does not have `*` anymore)

================
Comment at: test/CodeGen/WebAssembly/irreducible-cfg.ll:224
+; CHECK-NOT: br_table
+define hidden void @ps_hints_apply() #0 {
+entry:
----------------
- If this is without irreducibility, shouldn't this be in `non-irreducible-cfg.ll` below?
- Can we remove `#0`?

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58919