On 2013/11/20 09:45:24, danno wrote:
https://codereview.chromium.org/59023003/diff/260001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/59023003/diff/260001/src/hydrogen-instructions.cc#newcode3788
src/hydrogen-instructions.cc:3788: if (!forced_representation.IsNone()) {
On 2013/11/19 09:42:04, Dmitry Lomov (chromium) wrote:
> I am not entirely convinced this is a good change. In my original
patch, I
> explicitly created an (external + offset) => external instruction,
which is
the
> only case we handle downstream. Here it appears that we can handle a (r
+
smth)
> => r for all r, which is not true. wdyt?
I see your point, but I also really don't like the idea of two
instructions.
It
seems the Add should "automagically" do the right thing when one of it's
inputs
is an external pointer, since that's the special case you need to handle.
i.e.
why do you need to set the observed input representation in the first
place?
Shouldn't the instruction just do the right thing when this input is
External?
If not, there seems to be some missing machinery somewhere else.
Alternatively, since there are only two additional calls that are needed
in
this
path, can it be done separately from the constructor? i.e. remove the
input to
the constructor and have a "MarkAsExternalPointerAdd" method?
In any case, let's discuss in person.
Turns out, this change has been masquerading some other, since fixed, bug
in my
changes.
Changes to HAdd::RepresentationsFromInputs and
HAdd::RequiredInputRepresentation
are quite enough.
https://codereview.chromium.org/59023003/
--
--
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/groups/opt_out.
