Reviewers: rossberg, Message: On 2014/08/06 13:04:08, rossberg wrote:
https://codereview.chromium.org/425003004/diff/20001/src/compiler/js-typed-lowering.cc
File src/compiler/js-typed-lowering.cc (right):
https://codereview.chromium.org/425003004/diff/20001/src/compiler/js-typed-lowering.cc#newcode518
src/compiler/js-typed-lowering.cc:518: if (input_type->Is(Type::Number())) {Why is this duplicated?
Oversight. I think I had a special case for Int32 there before. https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc
File src/compiler/simplified-lowering.cc (left):
https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#oldcode322
src/compiler/simplified-lowering.cc:322: case IrOpcode::kLoadField: Why did these cases disappear?
This method only lowers Change operators now. Loads and stores of fields and elements are handled in the VisitNode() now. https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc
File src/compiler/simplified-lowering.cc (right):
https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode32
src/compiler/simplified-lowering.cc:32: enum Phase {
I wish we could somehow factor this using some form of parameterisation,
and
not
a mode flag... :(
As discussed, there are a couple ways to do parameterization.1. With a template parameter, either at the class or the method level. The code
duplication that results concerns me a little, since the entire switch andeverything would be duplicated. If at the method level, then it has to appear through the entire chain of Visit* methods. If at the class level, then there is
even more code duplication.2. With a boolean flag. If at the class level, then you have what I have now. If you have it at the method level, then you have to pass it down through all the
layers.3. With virtual methods for places where the behavior is different, either on a
passed in object, or on subclass of this class. https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode60
src/compiler/simplified-lowering.cc:60: RepTypeUnion output : 16; // Outputtype of the node. Why is this bitset larger than the other?
Doesn't have to be. I just steal 2 bits for the traversal state. https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode232
src/compiler/simplified-lowering.cc:232: // Helpers for specific types of binops. Nit: not sure these are worth it, but I leave it up to you.
I think it helps reduce the duplication in the big-ole switch. https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode247
src/compiler/simplified-lowering.cc:247: int values =
node->op()->InputCount();
Doesn't this include non-values?
It was correct but it's clearer now with OperatorProperties. https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode252
src/compiler/simplified-lowering.cc:252: ProcessInput(node, iter.index(),
values
> 0 ? use : 0); Oh, this breaks the node properties abstraction quite nastily.
Well yeah if it was a proper abstraction it could tell the kinds of the edges
it's iterating over. https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode310
src/compiler/simplified-lowering.cc:310: return VisitLeaf(node, rBit); Nit: could combine these two cases.
Done. https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode312
src/compiler/simplified-lowering.cc:312: return VisitLeaf(node, rBit); Why rBit?
It shouldn't really matter, as they are leaf nodes and shouldn't flow into anything except as a control or effect dependency, whose representations _should_ be ignored. *fingers crossed* https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode343
src/compiler/simplified-lowering.cc:343: //------------------------------------------------------------------ Nit: weird indentation
clang-format'd. Thaz how it iz https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode369
src/compiler/simplified-lowering.cc:369: // BooleanNot(x: rTagged) => WordEqual(x, #false) Wait, why is BooleanNot overloaded on JS Booleans?
It's representation independent so that word32 / float64 comparisons don't have
to make a round trip to tagged when flowing through BooleanNot to a branch. https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode446
src/compiler/simplified-lowering.cc:446: // Propage a type to the input, but
not
a representation. Typo: Propagate
Done. https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode552
src/compiler/simplified-lowering.cc:552: // We use unsigned int32 as the
output
type for shift right, thoughThis comment is confusing. Unsigned seems adequate here. Is this a bogus copy
&
paste from the next case, perhaps?
What do you mean? The output representation is word32 and the value is definitely an unsigned32. If it flows into a numbery thing, it needs to be converted to a positive number. https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode957
src/compiler/simplified-lowering.cc:957: void
SimplifiedLowering::Lower(Node*
node) {}
What's this still good for?
Because LoweringBuilder. Description:Implement representation selection as part of SimplifiedLowering. Representation
selection alsorequires inserting explicit representation change nodes to be inserted in the
graph. Such nodes are pure, but also need to be lowered to machine operators. They need to be scheduled first, to determine the control input for any branches inside. This CL requires extensive testing. More tests to follow. [email protected] BUG= Please review this at https://codereview.chromium.org/425003004/ SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge Affected files (+1636, -217 lines): M src/compiler/js-typed-lowering.cc M src/compiler/lowering-builder.cc M src/compiler/node-properties.h M src/compiler/representation-change.h M src/compiler/simplified-lowering.h M src/compiler/simplified-lowering.cc M test/cctest/compiler/call-tester.h M test/cctest/compiler/graph-builder-tester.h M test/cctest/compiler/test-changes-lowering.cc M test/cctest/compiler/test-simplified-lowering.cc -- -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev--- You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
