[PATCH] D48273: [WebAssembly] CFG stackify support for exception handling

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:126
+// contains instructions that should go before the marker, and AfterSet contains
+// ones that should go after the marker.
+static MachineBasicBlock::iterator
----------------
I think this idea of BeforeSet and AfterSet makes sense given the symmetry, but it was unclear on the first reading that only the BeforeSet is actually used other than for debugging (despite the ifdef and comment below). So this comment could maybe just have an additional ", but is only used for sanity checking" at the end.


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:294
+#ifndef NDEBUG
+    // All END_(BLOCK|LOOP|TRY) markers should be before the BLOCK.
+    if (MI.getOpcode() == WebAssembly::END_BLOCK ||
----------------
why is this only for debugging? edit: nevermind I figured it out. see comment abo.ve


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:307
+  // Local expression tree should go after the BLOCK.
+  for (auto I = Header->getFirstTerminator(), E = Header->begin(); I != E; --I)
+    if (WebAssembly::isChild(*std::prev(I), MFI))
----------------
I know LLVM style allows this complete lack of braces, but IMO using this many lines (control structures, etc) is just asking for bugs. Plus I find it difficult to read. Could we use the braces anyway in cases where the body is a nested expression?


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:539
+    auto TermPos = Header->getFirstTerminator();
+    if (TermPos == Header->end() || !WebAssembly::isRethrow(*TermPos))
+      for (const auto &MI : reverse(*Header))
----------------
Add braces here too.


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:670
 
 /// Insert LOOP and BLOCK markers at appropriate places.
+void WebAssemblyCFGStackify::placeMarkers(MachineFunction &MF) {
----------------
Update comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D48273