[PATCH] D11691: [WebAssembly] Add Relooper

[JF Bastien]

jfb added a comment.

I did a first quick LLVM-style-only pass. I stopped midway, because my comments would be the same, so I'll take another look after a code update.


================
Comment at: lib/Target/WebAssembly/Relooper.cpp:25
@@ +24,3 @@
+#include <string.h>
+#include <stdlib.h>
+#include <list>
----------------
Same here on C++ versions.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:51
@@ +50,3 @@
+  Condition = ConditionInit ? strdup(ConditionInit) : NULL;
+  Code = CodeInit ? strdup(CodeInit) : NULL;
+}
----------------
Could you just use a `std::string` here? Or do the `const char *` even need to be copied? If they don't then you could use a `StringRef`.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:81
@@ +80,3 @@
+                Target)); // cannot add more than one branch to the same target
+  BranchesOut[Target] = new Branch(Condition, Code);
+}
----------------
Can you instead use a `std::unique_ptr` in the container?

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:95
@@ +94,3 @@
+  for (unsigned i = 0; i < Shapes.size(); i++)
+    delete Shapes[i];
+}
----------------
Same here on `std::unique_ptr` in the container for the above two.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:141
@@ +140,3 @@
+      unsigned TotalCodeSize = 0;
+      for (BlockSet::iterator iter = Live.begin(); iter != Live.end(); iter++) {
+        Block *Curr = *iter;
----------------
With LLVM's containers you should be able to use range-based for loops here and in other places below.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:143
@@ +142,3 @@
+        Block *Curr = *iter;
+        TotalCodeSize += strlen(Curr->Code);
+      }
----------------
`std::string` or `StringRef` won't require O(n) `strlen` :-)

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:147
@@ +146,3 @@
+      BlockSet Removed;
+      // DebugDump(Live, "before");
+      for (BlockSet::iterator iter = Live.begin(); iter != Live.end(); iter++) {
----------------
Remove or uncomment. Same in a few other places below.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:156
@@ +155,3 @@
+        if (strlen(Original->Code) * (Original->BranchesIn.size() - 1) >
+            TotalCodeSize / 5)
+          continue; // if splitting increases raw code size by a significant
----------------
Could you make this a command-line option instead, so it can be tuned? See http://llvm.org/docs/CommandLine.html

================
Comment at: lib/Target/WebAssembly/Relooper.h:197
@@ +196,3 @@
+  enum ShapeType { Simple, Multiple, Loop };
+  ShapeType Type;
+
----------------
Would this be a better fit:
http://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html
?

================
Comment at: lib/Target/WebAssembly/Relooper.h:275
@@ +274,3 @@
+
+#if RELOOPER_DEBUG
+struct Debugging {
----------------
Instead use the `DEBUG` macro and LLVM's `DEBUG_TYPE`:
http://llvm.org/docs/ProgrammersManual.html#the-debug-macro-and-debug-option


Repository:
  rL LLVM

http://reviews.llvm.org/D11691