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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
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))
----------------
dschuff wrote:
> 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?
Yeah I used to place braces in this kind of cases too, but someone (not you) once told me to remove them in code review, and I guess that's when I started removing them :$ But I also think braces make code easy to read and less bug prone. Anyway I added the braces back.


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:443
+  // Compute the nearest common dominator of all unwind predecessors
+  MachineBasicBlock *Header = nullptr;
+  int MBBNumber = MBB.getNumber();
----------------
dschuff wrote:
> The fact that so many sections of this function are the same as or almost the same as `placeBlockMarker` kind of makes me wonder if we can do better about sharing the logic, although there are enough differences to make that challenging too. Probably worth thinking about, especially if we end up thinking of going toward a more explicit or IR-like representation of wasm control structures.
Hmm. Yes. In the current structure I also find it difficult to share the logics. Further, when determining `BeforeSet` and `AfterSet`, there are even more differences, partly because marker kinds are different, but also partly `try`/`end_try` marker placement is done lastly so it has to make sure the placement makes sense with respect to all previously placed `block`/`end_block`/`loop`/`end_loop` markers.


Repository:
  rL LLVM

https://reviews.llvm.org/D48273