Much better.
We could consider adding the logic when the HForceRepresentation is the
use, so
we could avoid creating the unnecessary HChange altogether, but I guess it's
unlikely that this situation would ever trigger, so it's not worth the
effort.
I do have some suggestions for simplification though.
https://codereview.chromium.org/303263010/diff/20001/src/hydrogen-representation-changes.cc
File src/hydrogen-representation-changes.cc (right):
https://codereview.chromium.org/303263010/diff/20001/src/hydrogen-representation-changes.cc#newcode54
src/hydrogen-representation-changes.cc:54: static bool
HChangeCanDeoptForUse(HChange* change, HValue* use, int use_index) {
I don't like that this function's name doesn't match what it computes.
E.g. a smi-to-int32 change never deopts, but on arm/ia32 this function
says it can.
What you're really interested in are unnecessary int32-to-smi
conversions, and I can't imagine any useful future generalizations. So
I'd suggest to:
- rename this function to IsNonDeoptingIntToSmiChange
- modify the comment at the call site to describe this specialized
purpose
- simplify the implementation a bit:
return from_rep.IsInt32() && to_rep.IsSmi() && SmiValuesAre32Bits();
https://codereview.chromium.org/303263010/diff/20001/src/hydrogen-representation-changes.cc#newcode55
src/hydrogen-representation-changes.cc:55: Representation change_rep =
change->RequiredInputRepresentation(0);
It's cleaner to use change->from()...
https://codereview.chromium.org/303263010/diff/20001/src/hydrogen-representation-changes.cc#newcode56
src/hydrogen-representation-changes.cc:56: Representation use_rep =
use->RequiredInputRepresentation(use_index);
...and change->to().
https://codereview.chromium.org/303263010/diff/20001/src/hydrogen-representation-changes.cc#newcode58
src/hydrogen-representation-changes.cc:58:
(change->CheckFlag(HValue::kUint32) == use->CheckFlag(HValue::kUint32))
&&
Representation changes are inserted before the uint32 analysis phase
(which sets kUint32 flags), so this should just be an ASSERT (with a
comment about phase ordering).
https://codereview.chromium.org/303263010/diff/20001/src/hydrogen-representation-changes.cc#newcode87
src/hydrogen-representation-changes.cc:87: HValue* parent =
HForceRepresentation::cast(value)->value();
nit: we don't use "parent" for instructions (they form a directed graph,
not a tree). Maybe "input"?
https://codereview.chromium.org/303263010/diff/20001/src/hydrogen-representation-changes.cc#newcode89
src/hydrogen-representation-changes.cc:89:
parent->RequiredInputRepresentation(0).Equals(req) &&
again,
s/parent->RequiredInputRepresentation(0)/HChange::cast(input)->from()/
https://codereview.chromium.org/303263010/
--
--
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.
