[PATCH] D11691: [WebAssembly] Add Relooper
Jonathan Roelofs
jonathan at codesourcery.com
Fri Jul 31 14:31:46 PDT 2015
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
More information about the llvm-commits
mailing list