On 2014/08/07 10:51:33, titzer wrote:
On 2014/08/06 13:04:08, rossberg wrote:
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 and
everything 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.
I'm actually warming up to using virtual methods here... :)
https://codereview.chromium.org/425003004/diff/20001/src/compiler/simplified-lowering.cc#newcode60
> src/compiler/simplified-lowering.cc:60: RepTypeUnion output : 16; //
Output
> type 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.
OK, but if RepTypeUnion fits into 14 bits, then why not make this 14 as
well?
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.
Maybe add a TODO. At least a comment documenting the assumption.
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*
Can't you use 0 then?
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
Dude, you're a big boy. Authorities only have to be respected when they
behave
sanely.
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, though
> This 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.
The comment essentially says "we use unsigned although there is no sign
bit". I
don't get the "although".
https://codereview.chromium.org/425003004/
--
--
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.
