[PATCH] D58919: WebAssembly: Irreducible control flow rewrite

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:259
+        // loop.)
+        BlockSet MutualLoopEntries;
+        for (auto *OtherLoopEntry : Graph.getLoopEntries()) {
----------------
Nit: How about renaming this to `EntriesForSingleLoop` or something and start by inserting `LoopEntry` first? (I don't really like the name `EntriesForSingleLoop`, but can't think of a better alternative now... I guess there can be better ones)
```
BlockSet EntriesForSingleLoop;
EntriesForSingleLoop.insert(LoopEntry);
for (auto *OtherLoopEntry : Graph.getLoopEntries()) {
  if (Graph.canReach(LoopEntry, OtherLoopEntry) &&
      Graph.canReach(OtherLoopEntry, LoopEntry))
    EntriesForSingleLoop.insert(OhterLoopEntry);
}

if (EntriesForSingleLoop.size() > 1) {
  makeSingleEntryLoop(EntriesForSingleLoop, Blocks, MF);
  ...
}
...

```
That way we don't need to `std::move` this later and don't need to check `if (OtherLoopEntry != LoopEntry)`. And in my opinion `if (EntriesForSingleLoop.size() > 1)` makes it clear that we take a multiple entry loop and transform it into a single entry loop.

This is just a matter of style preference, so if you don't like it, please ignore it.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:292
+        // to the graph are to add blocks on the way to a loop entry. As the
+        // loops are disjoint, that means we may only alter branches exiting
+        // another loop, which are ignored when recursing into that other loop
----------------
we may only alter branches 'in' existing..?


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:270
+        }
       }
+      // Only go on to actually process the inner loops when we are done
----------------
aheejin wrote:
> kripken wrote:
> > aheejin wrote:
> > > 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?
> > Yeah, it's somewhat debatable (is an inner loop an "inner loop" if there's a branch going into it from several levels up?), but yes, when we see irreducibility we handle it now, instead of leaving it for the inner loop. We have to, I think - if we can branch to it now, we need to fix up that branch now - we wouldn't see it later when we recurse and ignore the outside. At least that would require a very different approach I suppose (which might be more optimal in some cases).
> Is there a condition in the code that we only consider inner loops that have entries from several levels up? I don't think I found one. We just treat all blocks within the region the same way.
> 
> So, for example, an outer loop consists of blocks A, B, C, D, E, and F, and among them E and F comprises an inner loop. The outer loop has two entries: A and D from the blocks outside, and the inner loop also has two entries: E and F. E's enterer is C and F's enterer is D. In this case, can we solve this case in one pass with a single dispatch block that branches into all of A, D, E, and F?
> 
> I guess I don't understand the algorithm :( Sorry for that. Please correct me if I'm mistaken.
Oh, nevermind. I think I got it. :) Inner loop entries that are not reachable from outside will not be identified as loop entries when we examine the outer region, because those inner loop entries can also reach their preds, as long as the preds are not from outside and in an outer loop.


================
Comment at: test/CodeGen/WebAssembly/irreducible-cfg.ll:224
+; CHECK-NOT: br_table
+define hidden void @ps_hints_apply() #0 {
+entry:
----------------
aheejin wrote:
> kripken wrote:
> > aheejin wrote:
> > > - If this is without irreducibility, shouldn't this be in `non-irreducible-cfg.ll` below?
> > > - Can we remove `#0`?
> > * The other file has -O0, and this one doesn't specify an opt level (so it's -O2 I guess?), so I kept them separate.
> > * Removed `#0`.
> - You can use two different commands and check them separately within a single file.
> ```
> ; RUN: llc < %s -O0 -asm-verbose=false -verify-machineinstrs -disable-block-placement -wasm-disable-explicit-locals -wasm-keep-registers | FileCheck %s
> ; RUN: llc < %s -O2 -asm-verbose=false -verify-machineinstrs -disable-block-placement -wasm-disable-explicit-locals -wasm-keep-registers | FileCheck -check-prefix=OPT %s
> 
> ; OPT-NOT: br_table
> define hidden void @ps_hints_apply() {
>   ...
> }
> 
> ; CHECK-NOT: br_table
> define hidden i32 @_Z15fannkuch_workerPv(i8* %_arg) {
>   ...
> }
> ```
> 
> [[ https://llvm.org/docs/CommandGuide/FileCheck.html#the-filecheck-check-prefix-option | Here ]] is the manual for this `-check-prefix` option.
> 
> - Nit: Can we remove `hidden` too?
On second thought, wouldn't it better to just specify -O0 for all irreducibility tests? -O2 basically means we don't now what would happen to our carefully created irreducible CFG.


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

https://reviews.llvm.org/D58919