Fix OSR for nested loops.

Fix OSR for nested loops.

R=jkummerow@chromium.org
BUG=v8:2618
Committed: https://code.google.com/p/v8/source/detail?r=14202

[Jakob Kummerow, 2013-04-09 17:23:25]

Mostly nits.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/deoptimizer.h
File src/deoptimizer.h (right):

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/deoptimizer.h#n...
src/deoptimizer.h:227: // back to a normal interrupts.
nit: s/interrupts/interrupt/

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/deoptimizer.h#n...
src/deoptimizer.h:229: Code* interrupt_code,
nit: indentation

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/deoptimizer.h#n...
src/deoptimizer.h:233: // back to a normal interrupts.
nit: s/interrupts/interrupt/

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/deoptimizer.h#n...
src/deoptimizer.h:453: static const int kBackEdgeEntrySize = 2 * kIntSize +
kOneByteSize;
This constant belongs close to the definition of the BackEdgeEntry struct in
objects.h. Is there any reason you can't move it there, and remove it both here,
in objects.cc and in runtime.cc?

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/full-codegen.h
File src/full-codegen.h (right):

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/full-codegen.h#...
src/full-codegen.h:95: back_edges_(2, info->zone()),  // There's always at least
one.
nit: the comment is outdated, just remove it. (There's only a back edge if
there's a loop.)

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/full-codegen.h#...
src/full-codegen.h:827: // TODO(svenpanne) Rename this to something like
back_edges_ and rename
You can remove this TODO, since it describes exactly what this CL is doing.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/ia32/deoptimize...
File src/ia32/deoptimizer-ia32.cc (left):

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/ia32/deoptimize...
src/ia32/deoptimizer-ia32.cc:233: // ok: ...
This label got lost when you moved the comment. Please add it back, it is
meaningful.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/ia32/deoptimize...
src/ia32/deoptimizer-ia32.cc:244: ASSERT_EQ(kJnsInstruction, 
*(call_target_address - 3));
I have to say that I preferred this version of the checks, because here it's
trivially easy to convince yourself that exactly the right thing is being
checked.
Happy to discuss this if you prefer the new version :-)

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/objects.cc#newc...
src/objects.cc:9619: // If there is no back table, the "table start" will at or
after
nit: s/back/back edge/, s/will at/will be at/

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/objects.cc#newc...
src/objects.cc:9628: uint32_t ast_id = Memory::uint32_at(back_edge_cursor +
kIntSize);
typo: should be Memory::uint32_at(back_edge_cursor);

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/objects.h#newco...
src/objects.h:4507: // instruction stream where the stack check table starts.
nit: forgot one

[Yang, 2013-04-10 08:08:30]

Nits addressed. LGTY?

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/deoptimizer.h
File src/deoptimizer.h (right):

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/deoptimizer.h#n...
src/deoptimizer.h:227: // back to a normal interrupts.
On 2013/04/09 17:23:25, Jakob wrote:
> nit: s/interrupts/interrupt/

Done.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/deoptimizer.h#n...
src/deoptimizer.h:229: Code* interrupt_code,
On 2013/04/09 17:23:25, Jakob wrote:
> nit: indentation

Done.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/deoptimizer.h#n...
src/deoptimizer.h:233: // back to a normal interrupts.
On 2013/04/09 17:23:25, Jakob wrote:
> nit: s/interrupts/interrupt/

Done.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/deoptimizer.h#n...
src/deoptimizer.h:453: static const int kBackEdgeEntrySize = 2 * kIntSize +
kOneByteSize;
On 2013/04/09 17:23:25, Jakob wrote:
> This constant belongs close to the definition of the BackEdgeEntry struct in
> objects.h. Is there any reason you can't move it there, and remove it both
here,
> in objects.cc and in runtime.cc?

Done.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/full-codegen.h
File src/full-codegen.h (right):

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/full-codegen.h#...
src/full-codegen.h:95: back_edges_(2, info->zone()),  // There's always at least
one.
On 2013/04/09 17:23:25, Jakob wrote:
> nit: the comment is outdated, just remove it. (There's only a back edge if
> there's a loop.)

Done.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/full-codegen.h#...
src/full-codegen.h:827: // TODO(svenpanne) Rename this to something like
back_edges_ and rename
On 2013/04/09 17:23:25, Jakob wrote:
> You can remove this TODO, since it describes exactly what this CL is doing.

Done.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/ia32/deoptimize...
File src/ia32/deoptimizer-ia32.cc (left):

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/ia32/deoptimize...
src/ia32/deoptimizer-ia32.cc:233: // ok: ...
On 2013/04/09 17:23:25, Jakob wrote:
> This label got lost when you moved the comment. Please add it back, it is
> meaningful.

Done.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/ia32/deoptimize...
src/ia32/deoptimizer-ia32.cc:244: ASSERT_EQ(kJnsInstruction, 
*(call_target_address - 3));
On 2013/04/09 17:23:25, Jakob wrote:
> I have to say that I preferred this version of the checks, because here it's
> trivially easy to convince yourself that exactly the right thing is being
> checked.
> Happy to discuss this if you prefer the new version :-)

As discussed offline, this was refactored to be reused in VerifyInterruptCode.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/objects.cc#newc...
src/objects.cc:9619: // If there is no back table, the "table start" will at or
after
On 2013/04/09 17:23:25, Jakob wrote:
> nit: s/back/back edge/, s/will at/will be at/

Done.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/objects.cc#newc...
src/objects.cc:9628: uint32_t ast_id = Memory::uint32_at(back_edge_cursor +
kIntSize);
On 2013/04/09 17:23:25, Jakob wrote:
> typo: should be Memory::uint32_at(back_edge_cursor);

Done.

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/13811014/diff/6003/src/objects.h#newco...
src/objects.h:4507: // instruction stream where the stack check table starts.
On 2013/04/09 17:23:25, Jakob wrote:
> nit: forgot one

Yes. And many other places. I hope I caught all of them now.

[Jakob Kummerow, 2013-04-10 09:14:02]

lgtm

[Yang, 2013-04-10 09:24:52]

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