Collect type feedback for power-of-2 right operands in BinaryOps.

Collect type feedback for power-of-2 right operands in BinaryOps.

Improved --trace-ic output for unary/binary ops a bit on the way. Moved int32_t/uint32_t conversion helpers around.

R=jkummerow@chromium.org

Committed: http://code.google.com/p/v8/source/detail?r=14928

[Sven Panne, 2013-05-22 14:03:47]

Still incomplete (collecting the right thing on ia32 only), just to see if
things are going into the right direction...

[Sven Panne, 2013-05-23 11:37:06]

New shiny version, collecting type feedback on ia32 should be OK now. PTAL...

[Jakob Kummerow, 2013-05-23 12:05:43]

https://chromiumcodereview.appspot.com/15735005/diff/4001/src/code-stubs.h
File src/code-stubs.h (right):

https://chromiumcodereview.appspot.com/15735005/diff/4001/src/code-stubs.h#ne...
src/code-stubs.h:959: // Minor key encoding in 19 bits TTTRRRLLLSOOOOOOOMM.
Please update this comment.

https://chromiumcodereview.appspot.com/15735005/diff/4001/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/15735005/diff/4001/src/ic.cc#newcode2610
src/ic.cc:2610: (!previous_has_fixed_right_arg ||
I still disagree with this condition, as discussed offline previously. It should
be:
... &&
(previous_overall == BinaryOpIC::UNINITIALIZED ||
 (previous_has_fixed_right_arg && previous_fixed_right_arg_value == value));
Otherwise you'll go back to assuming a fixed right arg whenever the right arg
happens to be a power of two at a patching event even if you've seen non-fixed
right args before.

[Sven Panne, 2013-05-23 14:39:20]

Comments addressed. Platform ports done, one part is missing on ARM (see TODO),
but I'll leave that to our specialists. :-)

https://chromiumcodereview.appspot.com/15735005/diff/4001/src/code-stubs.h
File src/code-stubs.h (right):

https://chromiumcodereview.appspot.com/15735005/diff/4001/src/code-stubs.h#ne...
src/code-stubs.h:959: // Minor key encoding in 19 bits TTTRRRLLLSOOOOOOOMM.
On 2013/05/23 12:05:43, Jakob wrote:
> Please update this comment.

Done.

https://chromiumcodereview.appspot.com/15735005/diff/4001/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/15735005/diff/4001/src/ic.cc#newcode2610
src/ic.cc:2610: (!previous_has_fixed_right_arg ||
Ooops, forgot that. Fixed.

https://chromiumcodereview.appspot.com/15735005/diff/4001/src/ic.cc#newcode2663
src/ic.cc:2663: PrintF(" ((%s+%s",
While debugging this, I cleaned this trace output up a bit (and the one for
unary ops) to be more uniform and giving it ML-syntax. ;-)

[Jakob Kummerow, 2013-05-23 16:55:15]

ia32 and x64 look good, but what's with the TODO comment on ARM?

[Jakob Kummerow, 2013-06-03 09:49:30]

From our offline discussion:

https://chromiumcodereview.appspot.com/15735005/diff/26001/src/ia32/lithium-c...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/15735005/diff/26001/src/ia32/lithium-c...
src/ia32/lithium-codegen-ia32.cc:1272: Comment(";;; modi has_fixed_right_arg:%d
fixed_right_arg_value:%d",
This looks like a debugging leftover.

https://chromiumcodereview.appspot.com/15735005/diff/26001/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/15735005/diff/26001/src/ic.cc#newcode2575
src/ic.cc:2575: static bool TryToInt32(Handle<Object> object, int32_t* result) {
In deoptimizer.cc, we have a "static bool ObjectToInt32(Object* obj, int32_t*
value)", that's almost the same. Can we unify them?

[Sven Panne, 2013-06-03 12:37:02]

Addressed feedback so far, working on the last missing ARM piece...

https://codereview.chromium.org/15735005/diff/26001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/15735005/diff/26001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:1272: Comment(";;; modi has_fixed_right_arg:%d
fixed_right_arg_value:%d",
On 2013/06/03 09:49:30, Jakob wrote:
> This looks like a debugging leftover.

Done.

https://codereview.chromium.org/15735005/diff/26001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/15735005/diff/26001/src/ic.cc#newcode2575
src/ic.cc:2575: static bool TryToInt32(Handle<Object> object, int32_t* result) {
On 2013/06/03 09:49:30, Jakob wrote:
> In deoptimizer.cc, we have a "static bool ObjectToInt32(Object* obj, int32_t*
> value)", that's almost the same. Can we unify them?

Done: I've moved the conversion routines to int32_t/uint32_t to Object and reuse
them here and in deoptimizer.cc. There are more places which need some
refactoring, but not in this CL...

[Sven Panne, 2013-06-03 14:07:44]

PTAL again: Addressed feedback and offline discussions so far, so this CL is
ready for landing, LModI changes will be done separately.

[Jakob Kummerow, 2013-06-03 15:12:05]

LGTM with a suggestion.

https://codereview.chromium.org/15735005/diff/35001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/15735005/diff/35001/src/code-stubs.h#newcode942
src/code-stubs.h:942: FixedRightArgValueBits::is_valid(WhichPowerOf2(value));
nit: It would be nice to s/WhichPowerOf2/encode_arg_value/ here, however that
would create an infinite recursion with the ASSERT in encode_arg_value(). You'd
have to move the ASSERT to the call sites, i.e. into the two ctors. Your call.
(Yet another alternative would be to split encode_arg_value into an entry point
and and ..._internal function, but that seems overkill.)

[Rodolph Perfetta, 2013-06-03 16:33:17]

drive by comments

https://codereview.chromium.org/15735005/diff/35001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/15735005/diff/35001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:2230: __ VFPCompareAndSetFlags(d1, d8);
Wouldn't it be possible to compare to the already converted int32:

  __ cmp(right, Operand(fixed_right_arg_value());

[Sven Panne, 2013-06-04 07:48:09]

Landing...

https://codereview.chromium.org/15735005/diff/35001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/15735005/diff/35001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:2230: __ VFPCompareAndSetFlags(d1, d8);
On 2013/06/03 16:33:17, Rodolph Perfetta wrote:
> Wouldn't it be possible to compare to the already converted int32:
> 
>   __ cmp(right, Operand(fixed_right_arg_value());

Hmmm, unless I misunderstood things, 'right' doesn't contain the int32 value at
that point, it still contains the right operand, which could be a Smi or a
HeapNumber. On Intel things are different, the conversion macro instructions set
both floating register *and* a normal one. Because this doesn't appear to be the
case here, I did it the other way round (int=>double, then compare the double).

Nevertheless, if you see some way to improve this, I'll be happy to review the
change.

https://codereview.chromium.org/15735005/diff/35001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/15735005/diff/35001/src/code-stubs.h#newcode942
src/code-stubs.h:942: FixedRightArgValueBits::is_valid(WhichPowerOf2(value));
On 2013/06/03 15:12:05, Jakob wrote:
> nit: It would be nice to s/WhichPowerOf2/encode_arg_value/ here, however that
> would create an infinite recursion with the ASSERT in encode_arg_value().
You'd
> have to move the ASSERT to the call sites, i.e. into the two ctors. Your call.
> (Yet another alternative would be to split encode_arg_value into an entry
point
> and and ..._internal function, but that seems overkill.)

I had exactly the same thoughts when writing this, but I consciously ended up
with the current version: Moving an ASSERT to the call sites feels fundamentally
wrong, doing it 2 times feels even more wrong. Using an ..._internal function
seems to be overkill, furthermore the IsPowerOf2 and WhichPowerOf2 really belong
together. What one *really* wants here semantically is a function with type
'int32_t -> Maybe FixedRightArgValueBits'. C++ sucks...

Anyway, all this cr^H^H stuff should go away when we have a more general way of
attaching type feedback than our current glorious 25 bits. :-P

[Sven Panne, 2013-06-04 07:49:57]

Committed patchset #14 manually as r14928 (presubmit successful).