[PATCH] D58919: WebAssembly: Irreducible control flow rewrite

[Alon Zakai via Phabricator via llvm-commits]

kripken marked 11 inline comments as done.
kripken added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:64
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/Passes.h"
----------------
aheejin wrote:
> - Not sure what's happened, but it says "Context not available" on the diff on these lines. Could you rebase your CL onto the tip of the repo?
> - We usually include `WebAssembly.h` in every pass, and I guess we need to include  (Maybe it's not shown here because of the diff problem?)
Rebasing - hopefully that fixes the context issue.

Yeah, WebAssembly.h was included already, so might be the diff context problem.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:259
+        // loop.)
+        BlockSet MutualLoopEntries;
+        for (auto *OtherLoopEntry : Graph.getLoopEntries()) {
----------------
aheejin wrote:
> 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.
I agree the `move` is not nice. I think we can just add LoopEntry to MutualLoopEntries and simplify things that way, updating the patch with that, let me know what you think.


================
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
----------------
aheejin wrote:
> we may only alter branches 'in' existing..?
Thanks, I'll fix that.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:201
+    // from outside, we will traverse all the blocks in the loop.
+    BlockSet WorkList;
+    Blocks.insert(Entry);
----------------
aheejin wrote:
> kripken wrote:
> > aheejin wrote:
> > > Why is this worklist a set?
> > As a set, it won't have duplicate entries - so if we visit A that causes us to add C to the future work, and then visit B that also causes us to add C to the future work, then when we get to C we'll visit it once, and avoid visiting it again later (and not having anything to do the second time).
> > 
> > I haven't benchmarked this recently, but I remember it was beneficial in the Relooper.
> But isn't it possible that by the time you try to add C for the second time, the first C has been already erased from the set so you do the unnecessary work again? Usually what I've seen was making worklist as a vector and have a separate set called something like `Visited`, to which visited blocks are added and not erased.
The way you suggest is probably faster overall, I agree, I'll change it to that.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:371
+      DenseMap<MachineBasicBlock *, MachineBasicBlock *> Map;
+      for (auto *Entry : Pred->successors()) {
+        if (!Entries.count(Entry)) {
----------------
aheejin wrote:
> kripken wrote:
> > aheejin wrote:
> > > 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;
> > > ```
> > This is code that did not change in this patch (aside from names and clang-format), and I'm not sure I fully understand it (I think Dan wrote it?). But I don't think `Map[Entry]` can be overwritten? The shape of the code is
> > ```
> > DenseMap<MachineBasicBlock *, MachineBasicBlock *> Map;
> > for (auto *Entry : Pred->successors()) {
> >   [..]
> >   Map[Entry] = Split;
> > }
> > ```
> > 
> Ah right, it's not gonna be overwritten. So the code structure is
> ```
> for (MachineBasicBlock *Pred : AllPreds) {
>   DenseMap<MachineBasicBlock *, MachineBasicBlock *> Map;
>   for (auto *Entry : Pred->successors()) {
>     ...
>     Map[Entry] = Split;
>   }
> ```
> And in case we have entries E0 and E1, and there are predecessors P0 and P1, and the edges are like P0->E0, P0->E1, P1->E0, and P1->E1. So the both preds point to the both entries. In this case, each entry (E0 or E1) is processed twice in the inner for loop. Because `Map` is created within the outer loop, it's not gonna be overwritten, but `Split` for each E0 and E1 is gonna be created twice unnecessarily.
> 
> (I know it's not the part of the code that's changed in this CL; but I thought while we're writing the major part of the pass, maybe we can fix other things too. If you don't prefer that maybe I can submit that as a separate patch. Let me know which one you prefer.)
I'd prefer we do it separately since this patch already rewrites all the other code in this file, aside from that one unchanged function. Seems safer to change it later.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:390
+        BuildMI(*Split, Split->end(), DebugLoc(), TII.get(WebAssembly::BR))
+            .addMBB(Dispatch);
+        Split->addSuccessor(Dispatch);
----------------
aheejin wrote:
> aheejin wrote:
> > 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)
> Ping
Same issue as before - I'd prefer to not modify this one function in this patch (which changes everything aside from that one function), for safety.

(I did rename a bunch of variables, because I wanted to keep names consistent throughout the file - that seemed necessary in this patch.)


================
Comment at: test/CodeGen/WebAssembly/irreducible-cfg.ll:224
+; CHECK-NOT: br_table
+define hidden void @ps_hints_apply() #0 {
+entry:
----------------
aheejin wrote:
> 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.
Removed `hidden`, and changed to -O0 in irreducible-cfg. Yeah, that seems like the right thing to do for irreducibility tests.

I didn't want to change `irreducible-cfg-exceptions.ll`'s optimization level though because I'm not sure that wouldn't invalidate it.


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

https://reviews.llvm.org/D58919