Migrate instance of deprecated maps in HCheckMaps.

Migrate instance of deprecated maps in HCheckMaps.

Currently only direct map checks are supported. Otherwise only polymorphic cases with a generic fallback behave properly, regular polymorphic cases still need to be adapted.

R=danno@chromium.org

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

[Toon Verwaest, 2013-08-01 17:19:05]

PTAL.

This depends on https://chromiumcodereview.appspot.com/21065006/.

[danno, 2013-08-05 10:07:36]

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

https://codereview.chromium.org/21536003/diff/3001/src/hydrogen-instructions....
src/hydrogen-instructions.h:1089: virtual int CompareOperandCount() { return
OperandCount(); }
See comment below about storing the context explicitly.

https://codereview.chromium.org/21536003/diff/3001/src/hydrogen-instructions....
src/hydrogen-instructions.h:2870: class HCheckMaps: public
HTemplateInstruction<3> {
It seems to me this class would be much cleaner if you just stored the context
internally in a hand-allocated field of the class rather than one of the
built-in fields of the HTemplateInstruction<>

https://codereview.chromium.org/21536003/diff/3001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://codereview.chromium.org/21536003/diff/3001/src/ia32/lithium-ia32.h#ne...
src/ia32/lithium-ia32.h:2440: class LCheckMaps: public LTemplateInstruction<0,
2, 0> {
I think these are not needed given my previous comments.

[Toon Verwaest, 2013-08-05 11:32:13]

Addressed feedback. Given that the context is actually irrelevant for instance
migration, I removed the context slot from HCheckMaps, and clobbered the context
register for the runtime call with smi-0.

https://chromiumcodereview.appspot.com/21536003/diff/3001/src/hydrogen-instru...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/21536003/diff/3001/src/hydrogen-instru...
src/hydrogen-instructions.h:1089: virtual int CompareOperandCount() { return
OperandCount(); }
On 2013/08/05 10:07:36, danno wrote:
> See comment below about storing the context explicitly.

Done.

https://chromiumcodereview.appspot.com/21536003/diff/3001/src/hydrogen-instru...
src/hydrogen-instructions.h:2870: class HCheckMaps: public
HTemplateInstruction<3> {
On 2013/08/05 10:07:36, danno wrote:
> It seems to me this class would be much cleaner if you just stored the context
> internally in a hand-allocated field of the class rather than one of the
> built-in fields of the HTemplateInstruction<>

Done.

[Toon Verwaest, 2013-08-05 13:41:56]

Added platform ports, PTAL again.

[danno, 2013-08-05 14:36:59]

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

https://chromiumcodereview.appspot.com/21536003/diff/18002/src/ia32/lithium-c...
src/ia32/lithium-codegen-ia32.cc:5801: Runtime::kMigrateInstance, 1, instr,
instr->context());
I think you can make the code much smaller and simplify things even more if you
don't use CallRuntimeFromDeferred here at all. Perhaps just a new version called
CallRuntimeFromDeferredNoContext that just does:

__ xor (esi, esi)
__ CallRuntimeSaveDoubles(id);
__ RecordSafepointWithRegisters(instr->pointer_map(), argc,
Safepoint::kNoLazyDeopt);

https://chromiumcodereview.appspot.com/21536003/diff/18002/src/ia32/lithium-i...
File src/ia32/lithium-ia32.cc (right):

https://chromiumcodereview.appspot.com/21536003/diff/18002/src/ia32/lithium-i...
src/ia32/lithium-ia32.cc:2072: context = UseConstant(clobber_value);
I don't think you need any of this new logic, do you? The context register isn't
used, the code can stay the same, right?

https://chromiumcodereview.appspot.com/21536003/diff/18002/src/x64/lithium-co...
File src/x64/lithium-codegen-x64.cc (right):

https://chromiumcodereview.appspot.com/21536003/diff/18002/src/x64/lithium-co...
src/x64/lithium-codegen-x64.cc:4976: class DeferredInstanceMigration: public
LDeferredCode {
Do you want this class here or in the outer scope somewhere? It seems like you
might want to use it for other stuff at some point. Otherwise, it probably
should just be called DeferredCheckMaps.

[Toon Verwaest, 2013-08-05 15:03:26]

Addressed comments.

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

https://chromiumcodereview.appspot.com/21536003/diff/18002/src/ia32/lithium-c...
src/ia32/lithium-codegen-ia32.cc:5801: Runtime::kMigrateInstance, 1, instr,
instr->context());
On 2013/08/05 14:36:59, danno wrote:
> I think you can make the code much smaller and simplify things even more if
you
> don't use CallRuntimeFromDeferred here at all. Perhaps just a new version
called
> CallRuntimeFromDeferredNoContext that just does:
> 
> __ xor (esi, esi)
> __ CallRuntimeSaveDoubles(id);
> __ RecordSafepointWithRegisters(instr->pointer_map(), argc,
> Safepoint::kNoLazyDeopt);

Done.

https://chromiumcodereview.appspot.com/21536003/diff/18002/src/ia32/lithium-i...
File src/ia32/lithium-ia32.cc (right):

https://chromiumcodereview.appspot.com/21536003/diff/18002/src/ia32/lithium-i...
src/ia32/lithium-ia32.cc:2072: context = UseConstant(clobber_value);
On 2013/08/05 14:36:59, danno wrote:
> I don't think you need any of this new logic, do you? The context register
isn't
> used, the code can stay the same, right?

Done.

https://chromiumcodereview.appspot.com/21536003/diff/18002/src/x64/lithium-co...
File src/x64/lithium-codegen-x64.cc (right):

https://chromiumcodereview.appspot.com/21536003/diff/18002/src/x64/lithium-co...
src/x64/lithium-codegen-x64.cc:4976: class DeferredInstanceMigration: public
LDeferredCode {
On 2013/08/05 14:36:59, danno wrote:
> Do you want this class here or in the outer scope somewhere? It seems like you
> might want to use it for other stuff at some point. Otherwise, it probably
> should just be called DeferredCheckMaps.

Done.

[danno, 2013-08-05 15:10:48]

lgtm

[Toon Verwaest, 2013-08-05 16:42:50]

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