[PATCH] D81909: [WebAssembly] Fix bug in FixBrTables and use branch analysis utils

[Thomas Lively via Phabricator via llvm-commits]

tlively marked an inline comment as done.
tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp:63
+           "Expected jump or fallthrough to br_table block");
+    MI.addOperand(MF, MachineOperand::CreateMBB(TBB));
   } else {
----------------
aheejin wrote:
> tlively wrote:
> > aheejin wrote:
> > > tlively wrote:
> > > > tlively wrote:
> > > > > aheejin wrote:
> > > > > > Is it possible that both `TBB` and `FBB` are true and `FBB == MBB`?
> > > > > Yes, here's what's possible.
> > > > > 
> > > > > '_' is nullptr, 'J' is the jump table block (aka MBB), 'D' is the default block
> > > > > 
> > > > > TBB | FBB | meaning
> > > > > _ | _ | No default block, header falls through to jump table
> > > > > J | _ | No default block, header jumps to jump table
> > > > > D | _ | There is a default block, header falls through to jump table
> > > > > D | J | There is a default block, header jumps to either default or jump table
> > > > > 
> > > > Actually, I'll add this table as a comment. It seems useful.
> > > Yeah, I'm talking about the last case:
> > > D | J | There is a default block, header jumps to either default or jump table
> > > 
> > > In this case, not `TBB`, but `FBB` should be the default target. I don't think this code does it.
> > No, the condition that gets evaluated is whether the index is out of bounds, so the conditional jump (TBB) is to the default block and the unconditional jump (FBB) is to the jump table block (MBB). 
> I thought it could be more robust to handle both ways, because we were depending on the particular way the branch is generated in SelectionDAG now. But probably it's an overkill. :)
Ah gotcha. I'll add an assert to the `else` branch as well to make sure we catch this instead of miscompiling if the codegen ever changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81909