On 2014/08/07 13:26:47, rossberg wrote:
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".
I've reworded this and the other comment to be clearer (hopefully).
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.
