Lex, thanks. I'm fixing some stuff now, hopefully I can get some in before I
leave tomorrow evening.
-Ray
On Thu, Sep 17, 2009 at 2:11 PM, <sp...@google.com> wrote:
> Hi, Ray. Here's where I am at. Overall, the approach looks great.
> However, there are many details where I believe could be tightened up.
> Granted, I'm a little fuzzy on some parts; in such cases, let's at least
> work on the documentation to explain why things are. So, please update
> it and then let's look at an updated patch.
>
> Also, please add at least a few tests using the new compiler test
> infrastructure. I believe it should be possible to rig up a little
> assertUnflattens method that could be used like this:
>
> assertUnflattens("foo(t2);", "t1 = t2; foo(t2);");
>
>
>
>
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/8
> File dev/core/src/com/google/gwt/dev/jjs/impl/DeadCodeElimination.java
> (right):
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/8#newcode1781
> Line 1781: boolean execImpl(JNode node) {
> Intentional? It looks like it should be private. There are static
> methods to call from the outside.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/13
> File dev/core/src/com/google/gwt/dev/jjs/impl/Tracing.java (right):
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/13#newcode58
> Line 58: public static void clearStats() {
> These being statics will seem to cause trouble for anyone using the
> -localWorkers flag. We need to figure out a way not to need the
> statics.
>
> Besides, static state tend to lead to unwanted dependencies anyway.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/6
> File dev/core/src/com/google/gwt/dev/jjs/impl/Unflattener.java (right):
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/6#newcode113
> Line 113: orderCheck = OrderCheck.OK;
> I believe this else is only supposed to set orderCheck to OK if the
> variable is in fact the target variable. If it's some other variable,
> then the jury is still out.
>
> Based on that, the if-then should be:
>
> if (var == useDef.getVar()) {
> if (orderCheck == OrderCheck.CHECKING) {
> orderCheck = OrderCheck.OK;
> }
> } else {
> if (useDef.getSequenceNumber() < u.getSequenceNumber()) {
> maybeFail();
> }
> }
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/6#newcode159
> Line 159: didChange = changed || didChange;
> Per discussion in person, this should iterate over basic blocks, not
> over the whole program. Otherwise, there will be a lot of wasted
> iteration on blocks that have already been fixed up.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/3
> File
> dev/core/src/com/google/gwt/dev/jjs/impl/WorklistBasedOptimizer.java
> (right):
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/3#newcode103
> Line 103: workList.offer(v);
> We should revisit revisit the unflattener once the next round or two of
> optimization happen and look for opportunities to trim it down. For
> example, it might be that the unflattener only cares at all about
> single-def single-use variables, in which case it's not helpful to
> record a full list of all uses. As another example, based on how the
> unflattener is iterating, it might not help for
> DefinitionStatementPruner to bother trying to cascade a variable prune
> into more prunings.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/3#newcode205
> Line 205: * assignment.
> "... and update DefUse information accordingly."
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/3#newcode285
> Line 285: } else if (lhs instanceof JFieldRef) {
> I'm surprised by the field and array cases of this method. When would
> it matter that a statement modifies t.foo or t[foo]? I would think such
> statements should simply be ignored in this framework.
>
> For example, an assignment to t.foo does not invalidate anything assumed
> about t.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/3#newcode305
> Line 305: if (x.getOp() == JUnaryOperator.INC || x.getOp() ==
> JUnaryOperator.DEC) {
> Would CopyPropagate ever be called in this situation? That looks like
> an internal compiler error to me.
>
> As an example, if the unflattener said to inline some temp, but the temp
> is used in a ++ operation, then it would be a compiler error to even try
> to unflatten it.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/3#newcode346
> Line 346: public static boolean exec(JProgram program) {
> Intraprocedural is in a later patch. Can this exec method be moved to
> Intraprocedural? At any rate it doesn't compile as is.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/3#newcode356
> Line 356: protected Queue<UseDef> workList;
> This would work better as a LinkedHashSet, because contains() is called
> on it.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/12
> File dev/core/src/com/google/gwt/dev/jjs/impl/flow/ControlFlow.java
> (right):
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/12#newcode44
> Line 44: public class ControlFlow {
> How about something with "Builder" in the name? ControlFlowBuilder?
> ControlFlowGraphBuilder? Then you don't have to read the class comment
> to know what it does.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/12#newcode65
> Line 65: return false;
> If I read this class correctly, then the inter-block pred/succ links are
> currently incomplete. That would be a good thing to add to the class
> comment.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/12#newcode159
> Line 159: private BasicBlock createBlock(BasicBlock predBlock, JBlock
> stmts) {
> If stmts was a statement list rather than a block, it looks like this
> and the other overload of createBlock would get slightly shorter.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/10
> File dev/core/src/com/google/gwt/dev/jjs/impl/flow/ControlFlowGraph.java
> (right):
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/10#newcode35
> Line 35: private Set<BasicBlock> visited = new HashSet<BasicBlock>();
> Since visited is only used during the initial dfs() scan, it would be
> nicer if it were made an argument to dfs() and not a field.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/10#newcode37
> Line 37: private List<BasicBlock> order = new LinkedList<BasicBlock>();
> To be consistent with "postOrderIterator", maybe call this "postOrder"?
> Or, call the other one "orderIterator".
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/4
> File dev/core/src/com/google/gwt/dev/jjs/impl/flow/DataFlow.java
> (right):
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/4#newcode63
> Line 63: public class DataFlow {
> This class includes several unused variables and methods. They should
> be deleted. Findbugs will point them out. Your IDE probably can do so,
> as well, but I use Eclipse so am not sure.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/4#newcode163
> Line 163: // tryKillingVariables(x);
> Delete it if you don't want it.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/4#newcode248
> Line 248: testExpr = accept(testExpr);
> Dead store. Should just be accept(testExpr) without the assignment.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/4#newcode387
> Line 387: return super.visit(x, ctx);
> Because of the way GWT visitors work, all these calls to super.visit
> should instead simply return true. That will indicate that the
> sub-nodes of the argument should then be traversed.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/4#newcode418
> Line 418: // usages when on RHS, and this is a LHS fieldref usage
> Logically, such a use of a local really is a use. For example, given
> the code "t.foo = bar", t is used, and therefore t's definition cannot
> be removed. Similarly, if t is defined in one block, and then another
> block has "t.foo = bar", then t must be considered as used in multiple
> blocks. So, why stop recording uses in this context?
>
> To contrast, a direct assignment to a local is not a use. "t = bar" is
> much different from "t.foo = bar", and that wouldn't be a use of t.
>
> If that all sounds correct to you, then I believe I see a way to
> simplify this code. Remove the onAssignmentLHS field. Instead, have
> the visitor assume it's always on the RHS. The only LHS's that need
> special treatment are in the special case of a direct assignment to a
> local variable. In that specific case, don't have the visitor traverse
> the LHS. Instead, handle the LHS right there as appropriate.
>
> How does that strike you?
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/9
> File dev/core/src/com/google/gwt/dev/jjs/impl/flow/UseDef.java (right):
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/9#newcode88
> Line 88:
> Be sure to run "ant checkstyle" before commit.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/5
> File dev/core/src/com/google/gwt/dev/jjs/impl/flow/UseDefInfo.java
> (right):
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/5#newcode13
> Line 13: * change this template use File | Settings | File Templates.
> Automatic comment should be updated.
>
> http://gwt-code-reviews.appspot.com/64814/diff/1/5#newcode31
> Line 31: return new ArrayList(variableData.values());
> Raw type here. It's better to make it new ArrayList<UseDef>.
>
>
> http://gwt-code-reviews.appspot.com/64814
>
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
