[PATCH] D11691: [WebAssembly] Add Relooper

[Alon Zakai]

azakai added a comment.

Thanks for the feedback!

About clang-format, I did run it, but I didn't do so with `-style=LLVM`, I think that's why it didn't fix all the things you expected it to. However, it doesn't look like clang-format fixes  '{}'s when there's a single statement in the body of the if/for, so I have to do that manually.

I'll upload an updated diff.


================
Comment at: lib/Target/WebAssembly/Relooper.cpp:51
@@ +50,3 @@
+  Condition = ConditionInit ? strdup(ConditionInit) : NULL;
+  Code = CodeInit ? strdup(CodeInit) : NULL;
+}
----------------
jfb wrote:
> 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`.
This is actually code that will definitely be changed - we don't want a string of code, but instead some pointer to an LLVM structure. But I thought that removing it at this early time would make it harder to continue to work on this code. In other words, I left it here in order to make it clear what is to be replaced with proper LLVM content, when we get to that point.

================
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);
+}
----------------
jfb wrote:
> Can you instead use a `std::unique_ptr` in the container?
The unique pointer will be destroyed before we want the object to be destroyed, however (the object lives before and after it is used in this object).

The lifetime of all those is kept by the Relooper instance, when it is released, it frees everything inside it.

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:117
@@ +116,3 @@
+    void FindLive(Block *Root) {
+      BlockList ToInvestigate;
+      ToInvestigate.push_back(Root);
----------------
jroelofs wrote:
> I suspect `ToInvestigate` should be a `SmallPtrSet`, not a `std::list`.
I basically need a fifo list here, where I constantly take the first from the front and add to the back. Looking at SmallPtrSet, it seems unsuitable. Am I missing something?

================
Comment at: lib/Target/WebAssembly/Relooper.cpp:755
@@ +754,3 @@
+
+#define SHAPE_SWITCH(var, simple, multiple, loop)                              \
+  if (SimpleShape *Simple = Shape::IsSimple(var)) {                            \
----------------
jroelofs wrote:
> ewww... macros. Mind turning these into functions? In the case of `SHAPE_SWITCH`, that would mean passing it lambdas for `simple`, `multiple` and `loop`.
I worry about performance, since we do traverse the AST in optimization passes quite a bit.

Is there a guarantee that the LLVM optimizer would get rid of the lambda overhead here, if this were written that way?


Repository:
  rL LLVM

http://reviews.llvm.org/D11691