Hydrogen code stubs for vector-based ICs.

Hydrogen code stubs for vector-based ICs.

This patch finally allows running and passing tests with vector-based
Load and KeyedLoad ICs.

BUG=
R=jkummerow@chromium.org

Committed: https://chromium.googlesource.com/v8/v8/+/22302b51790a42231b67c18494cfbe17537e0d0b

[mvstanton, 2014-11-28 12:54:58]

mvstanton@chromium.org changed reviewers:
+ jkummerow@chromium.org

[mvstanton, 2014-11-28 12:56:29]

Hi Jakob,
Here is the rest of Load/KeyedLoadIC support for vectors. It includes:

* The HydrogenCodeStub dispatchers for the two ICs.
* Various Hydrogen changes to support them.
* IC handler changes to support the vector and slot, needed for MISS cases.

Thanks for the help!
--Michael

[mvstanton, 2014-11-28 13:00:01]

Patchset #1 (id:1) has been deleted

[Jakob Kummerow, 2014-12-02 08:43:07]

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2036: // We never return here, it is a tail call.
nit: I'd move this comment after the Add<...>.

If you feel you need to teach Hydrogen about the notion described in this
comment (e.g. because live ranges are an issue), the best approach is probably
modeled after HDeoptimize: end the block after the tail call instruction, emit a
goto to a new block, and update the HMarkUnreachableBlocksPhase to mark that
block as unreachable.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2046: // We never return here, it is a tail call.
same here

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2059: IfBuilder mono_checker(this);
Naming suggestion: if you name this guy "if_monomorphic", you'll get the very
natural "if_monomorphic.Then();" below. If you agree, consider similar namings
for other IfBuilders below.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2075: ? Add<HConstant>(static_cast<int32_t>(2))
nit: Drop the static_cast, it's cleaner.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2120: // Are we monomorphic?
nit: s/Are we foo/Is the IC in foo state/, and please mention polymorphic as
well.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2124: array_checker.If<HCompareMap>(feedback,
This map check bakes in the assumption that the feedback type sentinel is never
a Smi. Is that OK/intentional?

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2128: Add<HCheckHeapObject>(receiver);
Ugh. Hydrogen's ugly warts. HCheckHeapObject should be an iDef, and the
subsequent map load should have an explicit dependency on it. But that's not
this CL's business, and I guess by sheer luck the code just so happens to be
safe as it is...

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2214: // in that case right?
KeyedLoadIC currently doesn't support megamorphic state. For Smi keys, it can be
monomorphic, polymorphic, or generic. For non-Smi keys, it can be monomorphic or
generic. Yes, it's a mess. Future behavior TBD.

https://codereview.chromium.org/767743002/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/767743002/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:2411: call_mode_ = call_mode;
nit: for consistency, let's move both of these lines to the initializer section.
(You can keep the DCHECK as it is.)

https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler.cc
File src/ic/handler-compiler.cc (right):

https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler....
src/ic/handler-compiler.cc:135: if (FLAG_vector_ics &&
Can we re-use IC::ICUseVector here and below?

https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler....
src/ic/handler-compiler.cc:146: // miss occurs
nit: Trailing full stop.

https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler....
src/ic/handler-compiler.cc:150: !reg.is(object_reg)) {
Uhm... you've pushed scratch2/scratch3 if object_reg == vector_reg. Now you're
popping them if object_reg != vector_reg. If that's intentional, then it's
confusing and deserves a comment.

https://codereview.chromium.org/767743002/diff/20001/src/ic/ia32/handler-comp...
File src/ic/ia32/handler-compiler-ia32.cc (right):

https://codereview.chromium.org/767743002/diff/20001/src/ic/ia32/handler-comp...
src/ic/ia32/handler-compiler-ia32.cc:81: __ add(esp, Immediate(2 *
kPointerSize));  // remove vector and slot
nit: Capitalization and punctuation, please.

https://codereview.chromium.org/767743002/diff/20001/src/ic/ia32/handler-comp...
src/ic/ia32/handler-compiler-ia32.cc:610: (kind() == Code::LOAD_IC || kind() ==
Code::KEYED_LOAD_IC)) {
Again, can we re-use IC::ICUseVector() instead of duplicating the kinds list?

https://codereview.chromium.org/767743002/diff/20001/src/ic/ia32/handler-comp...
src/ic/ia32/handler-compiler-ia32.cc:613: PushVectorAndSlot(scratch2(),
scratch3());
Why the case distinction? Pushing scratch registers isn't any faster than
pushing the real things, or is it? Do they contain useful values at this point?
Also, it's not evident why slot and vector being on the stack is related to
holder and receiver registers being identical. Can you please add that to the
comment (if you keep the distinction)?

[mvstanton, 2014-12-03 11:45:14]

Patchset #2 (id:40001) has been deleted

[mvstanton, 2014-12-03 11:48:31]

Hi Jakob,
Thx for the comments! I've made two larger changes:

* logic change in the mono/poly case for the dispatcher. I was checking array
length before it was necessary, because I know it will have 2 (load) or 3
(keyedload) elements no matter what.

* The confusing code in handlers has been improved. The special cases are now
confined to Interceptor handler compilation.

Thanks for the look,
--Michael

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2036: // We never return here, it is a tail call.
On 2014/12/02 08:43:06, Jakob wrote:
> nit: I'd move this comment after the Add<...>.
> 
> If you feel you need to teach Hydrogen about the notion described in this
> comment (e.g. because live ranges are an issue), the best approach is probably
> modeled after HDeoptimize: end the block after the tail call instruction, emit
a
> goto to a new block, and update the HMarkUnreachableBlocksPhase to mark that
> block as unreachable.

Thanks, sounds like a good idea. I tried approximating it by just adding Deopts
after these tail calls, but it didn't reduce spill slots or their usage. I'll
tackle this in a follow-on CL.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2046: // We never return here, it is a tail call.
On 2014/12/02 08:43:06, Jakob wrote:
> same here

Done.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2059: IfBuilder mono_checker(this);
On 2014/12/02 08:43:06, Jakob wrote:
> Naming suggestion: if you name this guy "if_monomorphic", you'll get the very
> natural "if_monomorphic.Then();" below. If you agree, consider similar namings
> for other IfBuilders below.

Done.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2075: ? Add<HConstant>(static_cast<int32_t>(2))
On 2014/12/02 08:43:06, Jakob wrote:
> nit: Drop the static_cast, it's cleaner.

Done.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2075: ? Add<HConstant>(static_cast<int32_t>(2))
On 2014/12/02 08:43:06, Jakob wrote:
> nit: Drop the static_cast, it's cleaner.

Done. I reformulated this code a bit too, noticing that I don't need to check
the array length in the monomorphic case, but only if we didn't find the map in
the first tuple of the array.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2120: // Are we monomorphic?
On 2014/12/02 08:43:06, Jakob wrote:
> nit: s/Are we foo/Is the IC in foo state/, and please mention polymorphic as
> well.

Done.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2124: array_checker.If<HCompareMap>(feedback,
On 2014/12/02 08:43:06, Jakob wrote:
> This map check bakes in the assumption that the feedback type sentinel is
never
> a Smi. Is that OK/intentional?

Yes indeed, that is why we have a pile of symbols to distinguish different
states. This convention was introduced for the CallIC last summer to improve
performance.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2128: Add<HCheckHeapObject>(receiver);
On 2014/12/02 08:43:06, Jakob wrote:
> Ugh. Hydrogen's ugly warts. HCheckHeapObject should be an iDef, and the
> subsequent map load should have an explicit dependency on it. But that's not
> this CL's business, and I guess by sheer luck the code just so happens to be
> safe as it is...

Oh yeah, creepy. Okay, I've turned the HCheckHeapObject into a
HCheckSmiAndBranch, handling the MISS in the else case.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2214: // in that case right?
On 2014/12/02 08:43:06, Jakob wrote:
> KeyedLoadIC currently doesn't support megamorphic state. For Smi keys, it can
be
> monomorphic, polymorphic, or generic. For non-Smi keys, it can be monomorphic
or
> generic. Yes, it's a mess. Future behavior TBD.

Wow...the overloaded megamorphic_stub() method in ic.h led me into this error -
I ended up providing megamorphic support for keyed loads :p. But I want simple
and basic correctness, so I'm withdrawing that support for now. If it comes in
handy I can bring it back in a follow-on CL.

https://codereview.chromium.org/767743002/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/767743002/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:2411: call_mode_ = call_mode;
On 2014/12/02 08:43:07, Jakob wrote:
> nit: for consistency, let's move both of these lines to the initializer
section.
> (You can keep the DCHECK as it is.)

Done.

https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler.cc
File src/ic/handler-compiler.cc (right):

https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler....
src/ic/handler-compiler.cc:135: if (FLAG_vector_ics &&
On 2014/12/02 08:43:07, Jakob wrote:
> Can we re-use IC::ICUseVector here and below?

Done.

https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler....
src/ic/handler-compiler.cc:146: // miss occurs
On 2014/12/02 08:43:07, Jakob wrote:
> nit: Trailing full stop.

Done.

https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler....
src/ic/handler-compiler.cc:150: !reg.is(object_reg)) {
On 2014/12/02 08:43:07, Jakob wrote:
> Uhm... you've pushed scratch2/scratch3 if object_reg == vector_reg. Now you're
> popping them if object_reg != vector_reg. If that's intentional, then it's
> confusing and deserves a comment.

Okay, I've refactored this. The problem is that Frontend() is usually used in a
simple way, with object_reg == receiver(). But the interceptor case plays some
tricks with it. I removed all these contortions here in favor of confining them
to interceptor handler compilation, where they've been reformulated to make more
sense.

The general idea is that the interceptor wants to store the holder in the vector
register, but MISS can still occur during that period. So code that maintains
the vector and slot may need to shift where it stores them to scratch2() and
scratch3() during that time.

This story can now be read in a straight line as a part of interceptor handler
compilation.

https://codereview.chromium.org/767743002/diff/20001/src/ic/ia32/handler-comp...
File src/ic/ia32/handler-compiler-ia32.cc (right):

https://codereview.chromium.org/767743002/diff/20001/src/ic/ia32/handler-comp...
src/ic/ia32/handler-compiler-ia32.cc:81: __ add(esp, Immediate(2 *
kPointerSize));  // remove vector and slot
On 2014/12/02 08:43:07, Jakob wrote:
> nit: Capitalization and punctuation, please.

Done.

https://codereview.chromium.org/767743002/diff/20001/src/ic/ia32/handler-comp...
src/ic/ia32/handler-compiler-ia32.cc:610: (kind() == Code::LOAD_IC || kind() ==
Code::KEYED_LOAD_IC)) {
On 2014/12/02 08:43:07, Jakob wrote:
> Again, can we re-use IC::ICUseVector() instead of duplicating the kinds list?

Done.

https://codereview.chromium.org/767743002/diff/20001/src/ic/ia32/handler-comp...
src/ic/ia32/handler-compiler-ia32.cc:613: PushVectorAndSlot(scratch2(),
scratch3());
On 2014/12/02 08:43:07, Jakob wrote:
> Why the case distinction? Pushing scratch registers isn't any faster than
> pushing the real things, or is it? Do they contain useful values at this
point?
> Also, it's not evident why slot and vector being on the stack is related to
> holder and receiver registers being identical. Can you please add that to the
> comment (if you keep the distinction)?

My earlier lengthy comment and the code changes answer these good questions.

[Jakob Kummerow, 2014-12-03 13:22:18]

LGTM with nits.

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2128: Add<HCheckHeapObject>(receiver);
On 2014/12/03 11:48:30, mvstanton wrote:
> On 2014/12/02 08:43:06, Jakob wrote:
> > Ugh. Hydrogen's ugly warts. HCheckHeapObject should be an iDef, and the
> > subsequent map load should have an explicit dependency on it. But that's not
> > this CL's business, and I guess by sheer luck the code just so happens to be
> > safe as it is...
> 
> Oh yeah, creepy. Okay, I've turned the HCheckHeapObject into a
> HCheckSmiAndBranch, handling the MISS in the else case.

That's fine, but I didn't mean to imply you should do this. Using
HCheckHeapObject is fine. I was just ranting. Sorry if that confused you :-)

https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2214: // in that case right?
On 2014/12/03 11:48:30, mvstanton wrote:
> On 2014/12/02 08:43:06, Jakob wrote:
> > KeyedLoadIC currently doesn't support megamorphic state. For Smi keys, it
can
> be
> > monomorphic, polymorphic, or generic. For non-Smi keys, it can be
monomorphic
> or
> > generic. Yes, it's a mess. Future behavior TBD.
> 
> Wow...the overloaded megamorphic_stub() method in ic.h led me into this error
-
> I ended up providing megamorphic support for keyed loads :p. But I want simple
> and basic correctness, so I'm withdrawing that support for now. If it comes in
> handy I can bring it back in a follow-on CL.

Acknowledged.

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2091: IfBuilder if_correct_map(this);
nit: please don't shadow variables. (There's already an "if_correct_map" builder
in the outer scope.)

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2098: TailCallMiss(receiver, name, slot, vector,
keyed_load);
nit: drop this...

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2102: if_receiver_heap_object.Else();
...and this, and let all control flow paths fall through to a single
TailCallMiss() at the end.

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2192: // Check if the IC in generic state.
nit: missing "is"

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2204: // We never return here, it is a tail call.
nit: move this comment after the call please.

[mvstanton, 2014-12-04 10:28:20]

Patchset #3 (id:70001) has been deleted

[mvstanton, 2014-12-08 12:21:19]

Patchset #4 (id:110001) has been deleted

[mvstanton, 2014-12-08 12:21:28]

Patchset #3 (id:90001) has been deleted

[mvstanton, 2014-12-08 12:44:24]

Hi Jakob,
Okay patch set 3 contains the ports, along with the issues found/fixed while
making the ports.

Patch set 4 contains the response to your last round, thx for that.

Patch set 5 contains two changes that won't be checked in, but they allow the
rest of the porting team to verify that all tests pass as they make their port.
It consists of the flag being turned on, and one debug test disabled while I
debug why it fails.

I'll alert mips/x87 folks.

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2091: IfBuilder if_correct_map(this);
On 2014/12/03 13:22:17, Jakob wrote:
> nit: please don't shadow variables. (There's already an "if_correct_map"
builder
> in the outer scope.)

Done.

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2098: TailCallMiss(receiver, name, slot, vector,
keyed_load);
On 2014/12/03 13:22:17, Jakob wrote:
> nit: drop this...

Done.

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2102: if_receiver_heap_object.Else();
On 2014/12/03 13:22:17, Jakob wrote:
> ...and this, and let all control flow paths fall through to a single
> TailCallMiss() at the end.

Done. I removed these two and changed the caller BuildCodeStub() methods  to
handle the miss.

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2192: // Check if the IC in generic state.
On 2014/12/03 13:22:17, Jakob wrote:
> nit: missing "is"

wow, good eye :D

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2204: // We never return here, it is a tail call.
On 2014/12/03 13:22:17, Jakob wrote:
> nit: move this comment after the call please.

Done.

[mvstanton, 2014-12-08 14:57:05]

mvstanton@chromium.org changed reviewers:
+ akos.palfi@imgtec.com, weiliang.lin@intel.com

[mvstanton, 2014-12-08 14:57:06]

Akos, Weiliang, could you please take a look at mips and x87 ports?

This change makes flag --vector-ics functionally complete. There is a lot of
platform dependent code.

To make it easier, I've rebased the CL, and I've turned on the flag --vector-ics
(I won't check that part in). But this should allow you guys to test your
changes and make sure they work.

I appreciate the help, thank you, enjoy!
--Michael

[Jakob Kummerow, 2014-12-09 16:59:27]

Still LGTM, with one nit and a reminder about the flag.

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:2192: // Check if the IC in generic state.
On 2014/12/08 12:44:24, mvstanton wrote:
> On 2014/12/03 13:22:17, Jakob wrote:
> > nit: missing "is"
> 
> wow, good eye :D

Nah, just OCD ;-)

https://codereview.chromium.org/767743002/diff/150001/src/arm/lithium-codegen...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/767743002/diff/150001/src/arm/lithium-codegen...
src/arm/lithium-codegen-arm.cc:4037: // number of instructions for the constant
pool load.
nit: join this into the previous line (again below)

https://codereview.chromium.org/767743002/diff/150001/src/ic/arm/stub-cache-a...
File src/ic/arm/stub-cache-arm.cc (right):

https://codereview.chromium.org/767743002/diff/150001/src/ic/arm/stub-cache-a...
src/ic/arm/stub-cache-arm.cc:115: DCHECK(!AreAliased(receiver, name, scratch,
extra, extra2, extra3));
LGTM!

https://codereview.chromium.org/767743002/diff/190001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/767743002/diff/190001/src/flag-definitions.h#...
src/flag-definitions.h:251: DEFINE_BOOL(vector_ics, true, "support vector-based
ics")
Reminder: don't forget to undo this change (and the one in mjsunit.status)
before landing.

[mvstanton, 2014-12-12 13:56:43]

Committed patchset #10 (id:270001) manually as
22302b51790a42231b67c18494cfbe17537e0d0b (presubmit successful).