Re-land Crankshaft-generated KeyedLoad stubs.

Re-land Crankshaft-generated KeyedLoad stubs.

R=jkummerow@chromium.org

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

[danno, 2012-12-11 06:36:34]

PTAL. This is the patch on top of the monster CL that you already reviewed that
fixes known bugs that have been discovered in the last few days.

[Sven Panne, 2012-12-11 07:59:06]

Quick drive-by comment...

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/arm/macro-assem...
File src/arm/macro-assembler-arm.h (right):

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/arm/macro-assem...
src/arm/macro-assembler-arm.h:111: void IterateCompiledFrame(const
StandardFrame* frame, ObjectVisitor* v);
Shouldn't this be a method of StandardFrame? A naked top-level function always
looks a bit suspicious in an OO world...

Same for other platforms.

[Jakob Kummerow, 2012-12-11 14:05:54]

LGTM.

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/arm/macro-assem...
File src/arm/macro-assembler-arm.h (right):

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/arm/macro-assem...
src/arm/macro-assembler-arm.h:111: void IterateCompiledFrame(const
StandardFrame* frame, ObjectVisitor* v);
On 2012/12/11 07:59:06, Sven Panne wrote:
> Shouldn't this be a method of StandardFrame? A naked top-level function always
> looks a bit suspicious in an OO world...
> 
> Same for other platforms.

Well, the name "IterateCompiledFrame" expresses the notion that this is how you
iterate over a compiled frame, not over a StandardFrame, so renaming this to
StandardFrame::Iterate would not be appropriate (e.g. because we have
InternalFrame which also is-a StandardFrame but iterates differently). I'm not
opposed to moving it into the class without renaming it (i.e. calling it
StandardFrame::IterateCompiledFrame), but I don't feel strongly about it.

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

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/hydrogen-instru...
src/hydrogen-instructions.h:1947: SetOperandAt(1, typecheck != NULL ? typecheck
: value);
I could swear I've reviewed this change before... ah, yes:
https://codereview.chromium.org/11464027/

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/ia32/macro-asse...
File src/ia32/macro-assembler-ia32.h (right):

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/ia32/macro-asse...
src/ia32/macro-assembler-ia32.h:61: void IterateCompiledFrame(const
StandardFrame* frame, ObjectVisitor* v);
We #include frames.h above, which contains an identical declaration (except for
"const"). Can you get rid of this line by moving the constness into the
declaration in frames.h?

Same for the other platforms.

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/x64/lithium-cod...
File src/x64/lithium-codegen-x64.cc (right):

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/x64/lithium-cod...
src/x64/lithium-codegen-x64.cc:310: __ push(rsi);
oops :-)

[Sven Panne, 2012-12-11 14:19:27]

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/arm/macro-assem...
File src/arm/macro-assembler-arm.h (right):

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/arm/macro-assem...
src/arm/macro-assembler-arm.h:111: void IterateCompiledFrame(const
StandardFrame* frame, ObjectVisitor* v);
On 2012/12/11 14:05:54, Jakob wrote:
> On 2012/12/11 07:59:06, Sven Panne wrote:
> > Shouldn't this be a method of StandardFrame? A naked top-level function
always
> > looks a bit suspicious in an OO world...
> > 
> > Same for other platforms.
> 
> Well, the name "IterateCompiledFrame" expresses the notion that this is how
you
> iterate over a compiled frame, not over a StandardFrame, so renaming this to
> StandardFrame::Iterate would not be appropriate [...]

Well, that argument holds for the original signature as well: Using
StandardFrame is basically lying. :-) There seems to be something fishy in our
class hierarchy here, but I can live with a member function having a
warning-like name. Nevertheless, this is not solving the root of the problem...

[danno, 2012-12-11 14:26:49]

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/arm/macro-assem...
File src/arm/macro-assembler-arm.h (right):

https://chromiumcodereview.appspot.com/11528003/diff/2001/src/arm/macro-assem...
src/arm/macro-assembler-arm.h:111: void IterateCompiledFrame(const
StandardFrame* frame, ObjectVisitor* v);
There's not thing fishy about our class hierarchy. Stub frames are both
CompiledFrames and StandardsFrames. OptimizedFrames are CompiledFrames and
JavaScriptFrames. C++ makes expressing this painful, and the multiple
inheritance ban means that it will always look funny.

I have nothing against putting this method in StandardFrame with the "Compiled"
name, I'll do that.
On 2012/12/11 14:19:27, Sven Panne wrote:
> On 2012/12/11 14:05:54, Jakob wrote:
> > On 2012/12/11 07:59:06, Sven Panne wrote:
> > > Shouldn't this be a method of StandardFrame? A naked top-level function
> always
> > > looks a bit suspicious in an OO world...
> > > 
> > > Same for other platforms.
> > 
> > Well, the name "IterateCompiledFrame" expresses the notion that this is how
> you
> > iterate over a compiled frame, not over a StandardFrame, so renaming this to
> > StandardFrame::Iterate would not be appropriate [...]
> 
> Well, that argument holds for the original signature as well: Using
> StandardFrame is basically lying. :-) There seems to be something fishy in our
> class hierarchy here, but I can live with a member function having a
> warning-like name. Nevertheless, this is not solving the root of the
problem...

[danno, 2012-12-12 00:36:03]

Feedback addressed, I'll land this when the branch has been cut.

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

https://codereview.chromium.org/11528003/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:1947: SetOperandAt(1, typecheck != NULL ? typecheck
: value);
Landed the other patch first, rebased on it.
On 2012/12/11 14:05:54, Jakob wrote:
> I could swear I've reviewed this change before... ah, yes:
> https://codereview.chromium.org/11464027/

https://codereview.chromium.org/11528003/diff/2001/src/ia32/macro-assembler-i...
File src/ia32/macro-assembler-ia32.h (right):

https://codereview.chromium.org/11528003/diff/2001/src/ia32/macro-assembler-i...
src/ia32/macro-assembler-ia32.h:61: void IterateCompiledFrame(const
StandardFrame* frame, ObjectVisitor* v);
On 2012/12/11 14:05:54, Jakob wrote:
> We #include frames.h above, which contains an identical declaration (except
for
> "const"). Can you get rid of this line by moving the constness into the
> declaration in frames.h?
> 
> Same for the other platforms.

Done.