Refactoring Store ICs. A first step towards polymorphic store ICs.

Refactoring Store ICs. A first step towards polymorphic store ICs.

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

[Toon Verwaest, 2013-03-12 16:02:45]

PTAL

[Jakob Kummerow, 2013-03-13 13:27:25]

LGTM

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

https://codereview.chromium.org/12781004/diff/22001/src/stub-cache.cc#newcode...
src/stub-cache.cc:1616: ASSERT(kind == Code::STORE_IC || kind ==
Code::KEYED_STORE_IC ||
nit: You could avoid the duplication of constants here by making the
KEYED_STORE_IC case explicit below and adding an "else { UNREACHABLE(); }" in
the end.
You could also use a switch (with an UNREACHABLE default case), which I think
would look slightly cleaner, but would also be a bit more verbose due to
"break;" lines. Your choice.

https://codereview.chromium.org/12781004/diff/22001/src/stub-cache.cc#newcode...
src/stub-cache.cc:1632: void
StubCompiler::GenerateMissForceGeneric(MacroAssembler* masm,
Hm... fold this into the above? I have an idea for a pretty elegant approach:

void StubCompiler::GenerateMiss(MacroAssembler* masm, 
                                Builtins::Name miss_stub) {
  Handle<Code> code(masm->isolate->builtin(miss_stub));
  GenerateTailCall(masm, code);
}

And call sites would go from:
GenerateMissForceGeneric(masm, Code::KEYED_STORE_IC);
to:
GenerateMiss(masm, Builtins::kKeyedStoreIC_MissForceGeneric);
On second thought, that won't work so well for call sites where the kind is not
a constant, as in GenerateMiss(masm, kind()). Too bad.

https://codereview.chromium.org/12781004/diff/22001/src/stub-cache.cc#newcode...
src/stub-cache.cc:1688: Handle<Code> BaseStoreStubCompiler::GetICCode(Code::Kind
kind,
Hm... except for extra_state() vs. kNoExtraICState, this seems to be identical
to the BaseLoadStubCompiler version. I can see more cleanup coming in the future
(but it doesn't need to be now).

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

https://codereview.chromium.org/12781004/diff/22001/src/stub-cache.h#newcode818
src/stub-cache.h:818: : BaseStoreStubCompiler(isolate, strict_mode, registers())
{ }
nit: 4-space indent

https://codereview.chromium.org/12781004/diff/22001/src/stub-cache.h#newcode858
src/stub-cache.h:858: : BaseStoreStubCompiler(isolate, strict_mode,
registers()),
nit: 4-space indent

[Toon Verwaest, 2013-03-13 14:07:21]

Addressed feedback

https://chromiumcodereview.appspot.com/12781004/diff/22001/src/stub-cache.cc
File src/stub-cache.cc (right):

https://chromiumcodereview.appspot.com/12781004/diff/22001/src/stub-cache.cc#...
src/stub-cache.cc:1616: ASSERT(kind == Code::STORE_IC || kind ==
Code::KEYED_STORE_IC ||
On 2013/03/13 13:27:25, Jakob wrote:
> nit: You could avoid the duplication of constants here by making the
> KEYED_STORE_IC case explicit below and adding an "else { UNREACHABLE(); }" in
> the end.
> You could also use a switch (with an UNREACHABLE default case), which I think
> would look slightly cleaner, but would also be a bit more verbose due to
> "break;" lines. Your choice.

Done.

https://chromiumcodereview.appspot.com/12781004/diff/22001/src/stub-cache.cc#...
src/stub-cache.cc:1632: void
StubCompiler::GenerateMissForceGeneric(MacroAssembler* masm,
On 2013/03/13 13:27:25, Jakob wrote:
> Hm... fold this into the above? I have an idea for a pretty elegant approach:
> 
> void StubCompiler::GenerateMiss(MacroAssembler* masm, 
>                                 Builtins::Name miss_stub) {
>   Handle<Code> code(masm->isolate->builtin(miss_stub));
>   GenerateTailCall(masm, code);
> }
> 
> And call sites would go from:
> GenerateMissForceGeneric(masm, Code::KEYED_STORE_IC);
> to:
> GenerateMiss(masm, Builtins::kKeyedStoreIC_MissForceGeneric);
> On second thought, that won't work so well for call sites where the kind is
not
> a constant, as in GenerateMiss(masm, kind()). Too bad.

Done.

https://chromiumcodereview.appspot.com/12781004/diff/22001/src/stub-cache.cc#...
src/stub-cache.cc:1688: Handle<Code> BaseStoreStubCompiler::GetICCode(Code::Kind
kind,
Will do this in a later CL.

On 2013/03/13 13:27:25, Jakob wrote:
> Hm... except for extra_state() vs. kNoExtraICState, this seems to be identical
> to the BaseLoadStubCompiler version. I can see more cleanup coming in the
future
> (but it doesn't need to be now).

https://chromiumcodereview.appspot.com/12781004/diff/22001/src/stub-cache.h
File src/stub-cache.h (right):

https://chromiumcodereview.appspot.com/12781004/diff/22001/src/stub-cache.h#n...
src/stub-cache.h:818: : BaseStoreStubCompiler(isolate, strict_mode, registers())
{ }
On 2013/03/13 13:27:25, Jakob wrote:
> nit: 4-space indent

Done.

https://chromiumcodereview.appspot.com/12781004/diff/22001/src/stub-cache.h#n...
src/stub-cache.h:858: : BaseStoreStubCompiler(isolate, strict_mode,
registers()),
On 2013/03/13 13:27:25, Jakob wrote:
> nit: 4-space indent

Done.

[Toon Verwaest, 2013-03-13 14:11:14]

Committed patchset #6 manually as r13934 (presubmit successful).