[PATCH] D11691: [WebAssembly] Add Relooper

[JF Bastien]

jfb added inline comments.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:1
@@ +1,2 @@
+//===-- Relooper.h - Top-level interface for WebAssembly  ----*- C++ -*-===//
+//
----------------
.cpp

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:31
@@ +30,3 @@
+#include <llvm/Support/CommandLine.h>
+#include <llvm/ADT/SmallPtrSet.h>
+
----------------
You should put `llvm/` before system `#include`s:
http://llvm.org/docs/CodingStandards.html#include-style
Same for `Relooper.h`.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:74
@@ +73,3 @@
+  free(static_cast<void *>(const_cast<char *>(BranchVar)));
+  for (auto iter : ProcessedBranchesOut) {
+    delete iter.second;
----------------
In many cases you'll want to use `for (const auto &&` or `for (const auto &`. In this case it's just copying a `std::pair` of pointers so it's not that bad, but in general you can get some pretty unexpected results with just `auto`.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:95
@@ +94,3 @@
+Relooper::~Relooper() {
+  for (unsigned i = 0; i < Blocks.size(); i++)
+    delete Blocks[i];
----------------
LLVM style capitalizes the `I`. I know...
But this could be a range-based for loop anyways :-)
Or even better, some `std::unique_ptr`.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:329
@@ +328,3 @@
+            : IndependentGroups(IndependentGroupsInit) {}
+        void InvalidateWithChildren(Block *New) { // TODO: rename New
+          BlockList ToInvalidate;   // Being in the list means you
----------------
`New` is OK.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:331
@@ +330,3 @@
+          BlockList ToInvalidate;   // Being in the list means you
+                                    // need to be invalidated
+          ToInvalidate.push_back(New);
----------------
You should probably move some of these comments tot he line above, since it looks cramped with 80-column.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:482
@@ +481,3 @@
+      // The multiple has been created, we can decide how to implement it
+      if (Multiple->InnerMap.size() >= 10) {
+        Multiple->UseSwitch = true;
----------------
Should this also be configurable?

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:654
@@ +653,3 @@
+#define SHAPE_SWITCH(var, simple, multiple, loop) {                            \
+  if (SimpleShape *Simple = dyn_cast<SimpleShape>(var)) {                      \
+    (void) Simple;                                                             \
----------------
`switch (var->getKind())` with `default: llvm_unreachable(...);` ?

================
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)
----------------
No guarantees, but we're within yelling distance of optimizer people who can fix LLVM's performance :)


Repository:
  rL LLVM

http://reviews.llvm.org/D11691