[PATCH] D11691: [WebAssembly] Add Relooper

[JF Bastien]

jfb added inline comments.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:52
@@ +51,3 @@
+
+Branch::Branch(const char *ConditionInit, const char *CodeInit)
+    : Ancestor(nullptr), Labeled(true) {
----------------
You mean in this patch, or a later one? It would be good to have a `FIXME` here.

I'm mostly asking because ew `strdup` and `free` :-)

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:82
@@ +81,3 @@
+  assert(
+      !contains(BranchesOut,
+                Target)); // cannot add more than one branch to the same target
----------------
Hmm, so the lifetime starts in `BranchesOut` below, but ends elsewhere (`ToInvestigate`?) while `BranchesOut` doesn't live for as long?

It's just unusual / undesirable to have manual memory management for algorithms like this. I get confused. Could it maybe instead use a `BumpPtrAllocator`, which allows you to move the objects around but bounds the lifetime properly? Or do you actually need to delete things during relooping otherwise it would consume too much memory?

I'm asking without understanding the algorithm yet (or rather, I haven't read the code in a while). So I've got my style hat on, feel free to shoo it off if it looks silly.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:118
@@ +117,3 @@
+    BlockSet Live;
+
+    void FindLive(Block *Root) {
----------------
If it usually stay small then LLVM's `Small*` datastructures can just fit on the stack, and grow to the heap if needed. Nicer than `std::list` because of this. `SmallVector` can be a good fifo, but it's only suited to small things than don't usually grow bigger than your stack allocated quantity.


Repository:
  rL LLVM

http://reviews.llvm.org/D11691