Remove Reloc::Mode CODE_TARGET_CONTEXT

We need to know if a load, store or call IC is assumed
to be on the global object. Previously, this information
was stored in RelocInfo. A more logical place for this kind
of structural information is ExtraICState. Storing it there
makes it easier for us to gather type feedback from these
sites too.

R=verwaest@chromium.org

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

[mvstanton, 2013-12-02 15:01:39]

Hi Toon, here is the change we've been discussing. If it looks okay, I'll
address the ports.
Thx,
--Michael

[Toon Verwaest, 2013-12-02 15:33:57]

Looking good. More macros / lists would probably reduce some of the code.

Also we probably should keep contextual/strict unlinked: allow both
non-strict+contextual and strict+contextual; even if it's not visible from
javascript.

https://codereview.chromium.org/96083005/diff/60001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/96083005/diff/60001/src/builtins.cc#newcode1320
src/builtins.cc:1320: }
Please, as discussed, just generate these definitions using macros.

https://codereview.chromium.org/96083005/diff/60001/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/96083005/diff/60001/src/builtins.h#newcode139
src/builtins.h:139: IC::kContextualState)               \
Generate using macros.

[mvstanton, 2013-12-03 14:54:54]

Hi Toon,
Thanks for the comments, I've 
1) used macros in builtins[.h/.cc] to reduce verbosity.
2) made sure that contextual and strictmode are treated separately. So for
example we can have a StoreIC that is contextual but not strict mode.
3) Added a test where I take call, load and store ics through some states to
make sure the state bits with contextual are transferred correctly.
4) An assert in set_target() for StoreIC that the contextual bits as well as
strict mode bits are properly propagated.

Thanks, I'll let you have a first look then address the platform ports.
--Michael

[mvstanton, 2013-12-04 16:35:48]

Hi Toon,
Okay, per your verbal comments, I've moved loads and stores out of the builtins
into the stub cache. Thanks!
--Michael

[Toon Verwaest, 2013-12-04 18:10:51]

I like this change! Looking good. Added comments and nitty nits.

https://codereview.chromium.org/96083005/diff/160001/src/disassembler.cc
File src/disassembler.cc (right):

https://codereview.chromium.org/96083005/diff/160001/src/disassembler.cc#newc...
src/disassembler.cc:240: if (!code->needs_extended_extra_ic_state(kind) &&
This seems wrong. What about code->IsContextual() which internally first checks
whether it can be contextual (only true for load/store/call), and if so, decodes
it from the extra ic state?

https://codereview.chromium.org/96083005/diff/160001/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/96083005/diff/160001/src/full-codegen.cc#newc...
src/full-codegen.cc:453: is_classic_mode() ? kNonStrictMode : kStrictMode,
What about adding a strict_mode() helper?

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc#newcode496
src/ic.cc:496: // Is the site contextual or not?
That comment is a bit superfluous.

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc#newcode500
src/ic.cc:500: target->arguments_count(),
nit: I think you can join these 3 lines on 1 line.

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc#newcode518
src/ic.cc:518: Code* code =
target->GetIsolate()->stub_cache()->FindPreMonomorphicIC(
LoadIC::pre_monomorphic_stub(target->GetIsolate(), target->extra_ic_state()); ?

At least it would be nice to have a shorter call sequence here.

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc#newcode519
src/ic.cc:519: Code::LOAD_IC,
Join lines.

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc#newcode528
src/ic.cc:528: Code::STORE_IC,
Join lines.

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc#newcode1115
src/ic.cc:1115: Handle<Code> ic =
isolate->stub_cache()->ComputeLoad(UNINITIALIZED,
UNINITIALIZED on the next line, join with the mode computation.

https://codereview.chromium.org/96083005/diff/160001/src/ic.h
File src/ic.h (right):

https://codereview.chromium.org/96083005/diff/160001/src/ic.h#newcode125
src/ic.h:125: return contextual_mode();
Casting ContextualMode to bool? Please either return the mode or just make it
IsContextual().

https://codereview.chromium.org/96083005/diff/160001/src/ic.h#newcode270
src/ic.h:270: }
Do we really want this setter? Shouldn't that just be initialized in the
constructor?

https://codereview.chromium.org/96083005/diff/160001/src/stub-cache.cc
File src/stub-cache.cc (right):

https://codereview.chromium.org/96083005/diff/160001/src/stub-cache.cc#newcod...
src/stub-cache.cc:895: if (ic.contextual_mode() == NOT_CONTEXTUAL) {
Adding IsContextual is probably a good idea, if we use it like this.

https://codereview.chromium.org/96083005/diff/160001/src/stub-cache.h
File src/stub-cache.h (right):

https://codereview.chromium.org/96083005/diff/160001/src/stub-cache.h#newcode188
src/stub-cache.h:188: Code* FindCallInitialize(int argc, ContextualMode mode,
Code::Kind kind);
We should probably also reset calls to premonomorphic rather than initialize.
But meh, another CL.

https://codereview.chromium.org/96083005/diff/160001/test/mjsunit/context-cal...
File test/mjsunit/context-calls-maintained.js (right):

https://codereview.chromium.org/96083005/diff/160001/test/mjsunit/context-cal...
test/mjsunit/context-calls-maintained.js:30: function dispose_context() {
Maybe call this "clear_all_ics()" ?

[mvstanton, 2013-12-04 22:08:10]

Whew. PTAL.

https://codereview.chromium.org/96083005/diff/160001/src/disassembler.cc
File src/disassembler.cc (right):

https://codereview.chromium.org/96083005/diff/160001/src/disassembler.cc#newc...
src/disassembler.cc:240: if (!code->needs_extended_extra_ic_state(kind) &&
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> This seems wrong. What about code->IsContextual() which internally first
checks
> whether it can be contextual (only true for load/store/call), and if so,
decodes
> it from the extra ic state?

Done.

https://codereview.chromium.org/96083005/diff/160001/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/96083005/diff/160001/src/full-codegen.cc#newc...
src/full-codegen.cc:453: is_classic_mode() ? kNonStrictMode : kStrictMode,
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> What about adding a strict_mode() helper?

Done.

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc#newcode496
src/ic.cc:496: // Is the site contextual or not?
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> That comment is a bit superfluous.

Done.

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc#newcode500
src/ic.cc:500: target->arguments_count(),
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> nit: I think you can join these 3 lines on 1 line.

Done.

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc#newcode518
src/ic.cc:518: Code* code =
target->GetIsolate()->stub_cache()->FindPreMonomorphicIC(
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> LoadIC::pre_monomorphic_stub(target->GetIsolate(), target->extra_ic_state());
?
> 
> At least it would be nice to have a shorter call sequence here.

I can't quite do that here, because I can't create handles in the
LoadIC::Clear() function.

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc#newcode519
src/ic.cc:519: Code::LOAD_IC,
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> Join lines.

Done.

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc#newcode528
src/ic.cc:528: Code::STORE_IC,
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> Join lines.

Done.

https://codereview.chromium.org/96083005/diff/160001/src/ic.cc#newcode1115
src/ic.cc:1115: Handle<Code> ic =
isolate->stub_cache()->ComputeLoad(UNINITIALIZED,
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> UNINITIALIZED on the next line, join with the mode computation.

Done, and in the two functions below too.

https://codereview.chromium.org/96083005/diff/160001/src/ic.h
File src/ic.h (right):

https://codereview.chromium.org/96083005/diff/160001/src/ic.h#newcode125
src/ic.h:125: return contextual_mode();
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> Casting ContextualMode to bool? Please either return the mode or just make it
> IsContextual().

Yikes, that was unintentional, thx for catching. Fixed.

https://codereview.chromium.org/96083005/diff/160001/src/ic.h#newcode270
src/ic.h:270: }
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> Do we really want this setter? Shouldn't that just be initialized in the
> constructor?

I'll argue one more time to keep the setter, it allows me to keep
extra_ic_state_ as private instead of protected. There is a case in
CallIC::TryUpdateExtraICState() that uses this. I'd rather keep the setter and
then remove it when the CallIC is removed. Atavistic? :D

https://codereview.chromium.org/96083005/diff/160001/src/stub-cache.cc
File src/stub-cache.cc (right):

https://codereview.chromium.org/96083005/diff/160001/src/stub-cache.cc#newcod...
src/stub-cache.cc:895: if (ic.contextual_mode() == NOT_CONTEXTUAL) {
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> Adding IsContextual is probably a good idea, if we use it like this.

Done.

https://codereview.chromium.org/96083005/diff/160001/src/stub-cache.h
File src/stub-cache.h (right):

https://codereview.chromium.org/96083005/diff/160001/src/stub-cache.h#newcode188
src/stub-cache.h:188: Code* FindCallInitialize(int argc, ContextualMode mode,
Code::Kind kind);
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> We should probably also reset calls to premonomorphic rather than initialize.
> But meh, another CL.

Good point, I was wondering about that. But you are going to remove CallIC, and
you are best positioned to do the right thing.

https://codereview.chromium.org/96083005/diff/160001/test/mjsunit/context-cal...
File test/mjsunit/context-calls-maintained.js (right):

https://codereview.chromium.org/96083005/diff/160001/test/mjsunit/context-cal...
test/mjsunit/context-calls-maintained.js:30: function dispose_context() {
On 2013/12/04 18:10:52, Toon Verwaest wrote:
> Maybe call this "clear_all_ics()" ?

Done.

[Toon Verwaest, 2013-12-04 23:24:45]

LGTM with 1 comment. Make sure to check performance though.

https://codereview.chromium.org/96083005/diff/210001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/96083005/diff/210001/src/objects.cc#newcode10868
src/objects.cc:10868: if (!needs_extended_extra_ic_state(kind())) {
I still argue "needs_extended_extra_ic_state" has nothing to do with
IsContextual.
Please just check for LOAD_IC/CALL_IC/STORE_IC. All of them can be contextual.
Nothing else can be.

[mvstanton, 2013-12-05 13:08:54]

Thanks, Toon. 32bit perf appeared fine yesterday. I'll verify all platforms
today. Addressed your comment and uploaded MIPs platform.
--Michael

https://codereview.chromium.org/96083005/diff/210001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/96083005/diff/210001/src/objects.cc#newcode10868
src/objects.cc:10868: if (!needs_extended_extra_ic_state(kind())) {
On 2013/12/04 23:24:45, Toon Verwaest wrote:
> I still argue "needs_extended_extra_ic_state" has nothing to do with
> IsContextual.
> Please just check for LOAD_IC/CALL_IC/STORE_IC. All of them can be contextual.
> Nothing else can be.

No problem, done.

[mvstanton, 2014-01-07 14:14:55]

Committed patchset #12 manually as r18466 (presubmit successful).