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

[Derek Schuff via Phabricator via llvm-commits]

dschuff 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;
----------------
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?


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:694
+
+  // Linearing the control flow by placing TRY / END_TRY markers can create
+  // mismatches in unwind destinations. There are two kinds of mismatches we
----------------
s/Linearing/Linearizing


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:730
+  //    right after 'end_try', which means we extract the handler body out of
+  //    the catch block. We do this because this handler boy should be somewhere
+  //    branch-eable from the inner scope.
----------------
s/boy/body


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:764
+  // value created by catches. That's because we don't support yielding values
+  // from a block. in LLVM machine IR, even though it is supported by wasm.
+  // Delete unnecessary local.get/local.sets once yielding values from a block
----------------
remove the period after "block"


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:834
+  // there can be multiple cases:
+  // 1. We have -wasm-disable-explicit-locals set.
+  // 2. We don't have -wasm-disable-explicit-locals set.
----------------
It seems unfortunate that we have to care about whether explicit locals is turned on. Is that just because this pass runs after `createWebAssemblyExplicitLocals`? Does the order matter? e.g. would it be simpler to run it after CfgStackify? Or, could we just run it again if this pass changes anything?


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:869
+          !WebAssembly::mayThrow(MI))
+        continue;
+      SeenThrowableInstInBB = true;
----------------
`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?


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:943
+  assert(EHPadStack.empty());
+  // We don't any unwind destination mismatches to resolve.
+  if (UnwindDestToTryRanges.empty())
----------------
s/don't/don't have


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


================
Comment at: test/CodeGen/WebAssembly/cfg-stackify-eh-mismatch.ll:41
+
+define void @test0() personality i8* bitcast (i32 (...)* @__gxx_wasm_personality_v0 to i8*) {
+bb0:
----------------
Any weird corner cases it would be worthwhile to test for? Mixing both kinds of mismatches? A case with uwind-to-caller and block signature needed? Would multiple nesting of mismatches affect what happens?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D48345