[PATCH] D48345: [WebAssembly] Fix unwind destination mismatches in CFG stackify

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:75
 
+  // Newly appended BB at the end of the function for special purpose
+  MachineBasicBlock *AppendixBB = nullptr;
----------------
dschuff wrote:
> This comment is pretty vague. I guess this is one block for each function, and is now shared for creating a correct signature for fallthrough returns, and to use as a target for rethrows that need to unwind to the caller, but are trapped inside an outer try/catch?
Yes. Updated the comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:869
+          !WebAssembly::mayThrow(MI))
+        continue;
+      SeenThrowableInstInBB = true;
----------------
dschuff wrote:
> `continue` here will just skip to the next instruction, but the comment says it will skip the rest of the BB (which makes sense), should this be `break` instead? Or do we need to keep going to find the try/catch?
Right. It can't be `break` because we still need to process this part and update `EHPadStack`.
```
      if (MI.getOpcode() == WebAssembly::TRY)
        EHPadStack.pop_back();
      else if (MI.getOpcode() == WebAssembly::CATCH)
        EHPadStack.push_back(MI.getParent());
```


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:1012
+      }
+      if (MI.getOpcode() == WebAssembly::LOCAL_SET_EXCEPT_REF) {
+        assert(Catch && MI.getOperand(1).getReg() == Info.Reg);
----------------
dschuff wrote:
> these could be `else if`?
Changed to switch-case.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D48345