[PATCH] D11691: [WebAssembly] Add Relooper

[Alon Zakai]

azakai 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) {
----------------
jfb wrote:
> 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` :-)
Adding FIXMEs.

I think in a later patch makes sense - we still don't have a final plan for how to do it, and also it might end up being someone other than me that writes it.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:82
@@ +81,3 @@
+  assert(
+      !contains(BranchesOut,
+                Target)); // cannot add more than one branch to the same target
----------------
jfb wrote:
> 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.
Every branch starts out in BranchesOut. Then the algorithm step by step creates the AST, and while doing so, moves them one at a time to ProcessedBranchesOut. This is nice since we often need to iterate over  the not-yet-processed ones (so marking them with a flag and ignoring them would be less pleasant).

In theory a shared_ptr could be used in both of those (and in BranchesIn/ProcessedBranchesIn) but the overhead seems pointless to me.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:756
@@ +755,3 @@
+                  }
+                  Depth++; // this optimization increases depth, for us and all
+                           // our next chain (i.e., until this call returns)
----------------
jfb wrote:
> No guarantees, but we're within yelling distance of optimizer people who can fix LLVM's performance :)
Could we leave this for future investigation, then? In my experience with lambdas, the overhead is non-trivial (although I do love them for the code clarity, it's just that here, perf is quite important).


Repository:
  rL LLVM

http://reviews.llvm.org/D11691