[PATCH] D11691: [WebAssembly] Add Relooper

[Jonathan Roelofs]

jroelofs added a subscriber: jroelofs.
jroelofs added a comment.

Would be good to clang-format this before it goes in... I see a lot of places where '{}'s are used when there's a single statement in the body of the if/for.


================
Comment at: lib/Target/WebAssembly/Relooper.cpp:117
@@ +116,3 @@
+    void FindLive(Block *Root) {
+      BlockList ToInvestigate;
+      ToInvestigate.push_back(Root);
----------------
I suspect `ToInvestigate` should be a `SmallPtrSet`, not a `std::list`.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:371
@@ +370,3 @@
+        void InvalidateWithChildren(Block *New) { // TODO: rename New
+          BlockList ToInvalidate; // Being in the list means you need to be
+                                  // invalidated
----------------
I suspect `ToInvalidate` should be a `SmallPtrSet`, not a `std::list`.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:413
@@ +412,3 @@
+
+      BlockList Queue; // Being in the queue means we just added this item, and
+                       // we need to add its children
----------------
I suspect `Queue` should be a `SmallPtrSet`, not a `std::list`.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:467
@@ +466,3 @@
+        BlockSet &CurrGroup = IndependentGroups[*iter];
+        BlockList ToInvalidate;
+        for (BlockSet::iterator iter = CurrGroup.begin();
----------------
I suspect `ToInvalidate` should be a `SmallPtrSet`, not a `std::list`.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:755
@@ +754,3 @@
+
+#define SHAPE_SWITCH(var, simple, multiple, loop)                              \
+  if (SimpleShape *Simple = Shape::IsSimple(var)) {                            \
----------------
ewww... macros. Mind turning these into functions? In the case of `SHAPE_SWITCH`, that would mean passing it lambdas for `simple`, `multiple` and `loop`.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:798
@@ +797,3 @@
+    // by normal code
+    // execution is the same as the flow forces us to.
+    void RemoveUnneededFlows(Shape *Root, Shape *Natural = NULL,
----------------
looks like this comment got formatted poorly.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:823
@@ +822,3 @@
+                  // a direct: This would normally be  if (break?) { break; } ..
+                  // but if we
+                  // make sure to nest the else, we can save the break,  if
----------------
looks like this block of comment got formatted poorly.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:923
@@ +922,3 @@
+      if (First) {
+        Closure = new std::stack<Shape *>;
+      }
----------------
If the object it self has ownership of this stack instead of just the outermost call of this function, then the memory management would be much simpler.


Repository:
  rL LLVM

http://reviews.llvm.org/D11691