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#newcode240
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#newcode453
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#newcode895
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-calls-maintained.js
File test/mjsunit/context-calls-maintained.js (right):
https://codereview.chromium.org/96083005/diff/160001/test/mjsunit/context-calls-maintained.js#newcode30
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.
https://codereview.chromium.org/96083005/
--
--
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/groups/opt_out.
