Remove forced type changes when they can't deopt

Remove forced type changes when they can't deopt

Hydrogen attempts to force representation changes on certain operations in order
to deoptimise on the change rather than the operation. However, these forced
changes are often unnecessary on 64-bit platforms, and cause poor code
generation, so this patch makes some of them conditional on whether it's
possible for deoptimisation to occur in the change.

On ARM64, this prevents sequences like:
                  ;;; <@46,#89> smi-tag
0x7ff282c7f050   144  lsl x4, x4, #32
                  ;;; <@48,#90> smi-untag
0x7ff282c7f054   148  asr x5, x4, #32
                  ;;; <@50,#31> mul-const-i-s
0x7ff282c7f058   152  lsl w6, w5, #3

BUG=
R=jkummerow@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21818

[m.m.capewell, 2014-06-02 14:33:06]

There remains a similar tag/untag sequence caused by the forced representation
change for BuildUncheckedMonomorphicElementAccess(). However, I can't find a
good way to remove this, as the representation is unknown at the point that the
forced change is introduced.

[ulan, 2014-06-03 07:24:26]

Looks good. Adding Jakob to reviewers.

[Jakob Kummerow, 2014-06-03 08:08:49]

On 2014/06/02 14:33:06, m.m.capewell wrote:
> the representation is unknown at the point that the
> forced change is introduced.

That's exactly why putting this logic into the graph builder is the wrong
approach. Any operation that needs to check the representation of HValues should
be performed after the "infer representations" phase.

Usually I'd suggest to make this a part of canonicalization, but you'll probably
get better results by putting it directly into the "insert representation
changes" phase.

[m.m.capewell, 2014-06-09 14:27:52]

OK, took a while to get working, but here's a version that removes non-deopting
HChanges during the representation change phase. PTAL.

[Jakob Kummerow, 2014-06-12 10:16:50]

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-representat...
File src/hydrogen-representation-changes.cc (right):

https://codereview.chromium.org/303263010/diff/20001/src/hydrogen-representat...
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-representat...
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-representat...
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-representat...
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-representat...
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-representat...
src/hydrogen-representation-changes.cc:89:
parent->RequiredInputRepresentation(0).Equals(req) &&
again, s/parent->RequiredInputRepresentation(0)/HChange::cast(input)->from()/

[Jakob Kummerow, 2014-06-12 14:59:06]

LGTM. Do existing tests exercise this code path? If not, please add a test that
does (no need to assert that the optimization triggered, just ensure that
neither the compiler nor the generated code crashes).

[m.m.capewell, 2014-06-12 15:37:48]

Yes, this code is triggered regularly on 64-bit platforms; the output example
above is from the calculation of array size when allocating a new array.

[m.m.capewell, 2014-06-12 16:47:58]

Committed patchset #3 manually as r21818 (presubmit successful).