[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:378
+        MachineBasicBlock *Split = MF.CreateMachineBasicBlock();
+        MF.insert(Pred->isLayoutSuccessor(Entry)
+                      ? MachineFunction::iterator(Entry)
----------------
aheejin wrote:
> kripken wrote:
> > aheejin wrote:
> > > 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.
> > As before, this is Dan's code that I didn't change here (aside from variable names). I'm not sure what it does...
> (Because code parts have moved, this comment is not where it was anymore, so)
> The code is
> ```
> // This is a successor we need to rewrite.                               
> MachineBasicBlock *Split = MF.CreateMachineBasicBlock();                 
> MF.insert(Pred->isLayoutSuccessor(Entry)                                 
>               ? MachineFunction::iterator(Entry)                         
>               : MF.end(),                                                
>           Split);
> ```
> 
> I think the intention of the original code was, when we insert the `Split` which is to serve as a stepping stone between `Pred` and `Entry`, if currently `Pred` is right before `Entry` in the BB order, we want to insert `Split` between them, which makes a natural order. If not, we just append the new `Split` at the end of the function. I think this bug was there all along for years, and this was all newly created blocks are appended at the end.
> 
> I know this is not related to this CL, but if you prefer you can fix it maybe, or I can submit that as a separate patch.
Just letting you know, I was incorrect on this part; the code was correct. I realized that while I was working on D59462..


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58919