[turbofan] Implement OSR for outer loops.

[turbofan] Implement OSR for outer loops.

R=bmeurer@chromium.org
BUG=

Committed: https://crrev.com/159b14172f75ddb89e5daf2cceb9078f1f294916
Cr-Commit-Position: refs/heads/master@{#26020}

[Benedikt Meurer, 2014-12-18 11:21:22]

Nice, looks almost too easy. What is the test coverage for this?

https://codereview.chromium.org/809333002/diff/20001/src/compiler/arm/code-ge...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/809333002/diff/20001/src/compiler/arm/code-ge...
src/compiler/arm/code-generator-arm.cc:890: // TODO(titzer): put an abort into
the header of OSR functions on arm.
MacroAssembler::Abort()

https://codereview.chromium.org/809333002/diff/20001/src/compiler/arm64/code-...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/809333002/diff/20001/src/compiler/arm64/code-...
src/compiler/arm64/code-generator-arm64.cc:977: // TODO(titzer): put an abort
into the header of OSR functions on arm64.
MacroAssembler::Abort()

https://codereview.chromium.org/809333002/diff/20001/src/compiler/code-genera...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/809333002/diff/20001/src/compiler/code-genera...
src/compiler/code-generator.cc:270: // No OSR in Turbofan yet...
Remove this comment.

[titzer, 2015-01-09 14:17:39]

On 2014/12/18 11:21:22, Benedikt Meurer wrote:
> Nice, looks almost too easy. What is the test coverage for this?

I've added some test/mjsunit/compiler/*osr* tests that explicitly enable
--turbo-osr for outermost OSR loops. Additionally, I've added a few unit tests
that check the graph rewriting in test/cctest/compiler/test-osr.cc.

Also added documentation in src/compiler/osr.h
 
>
https://codereview.chromium.org/809333002/diff/20001/src/compiler/arm/code-ge...
> File src/compiler/arm/code-generator-arm.cc (right):
> 
>
https://codereview.chromium.org/809333002/diff/20001/src/compiler/arm/code-ge...
> src/compiler/arm/code-generator-arm.cc:890: // TODO(titzer): put an abort into
> the header of OSR functions on arm.
> MacroAssembler::Abort()
> 
>
https://codereview.chromium.org/809333002/diff/20001/src/compiler/arm64/code-...
> File src/compiler/arm64/code-generator-arm64.cc (right):
> 
>
https://codereview.chromium.org/809333002/diff/20001/src/compiler/arm64/code-...
> src/compiler/arm64/code-generator-arm64.cc:977: // TODO(titzer): put an abort
> into the header of OSR functions on arm64.
> MacroAssembler::Abort()
> 
>
https://codereview.chromium.org/809333002/diff/20001/src/compiler/code-genera...
> File src/compiler/code-generator.cc (right):
> 
>
https://codereview.chromium.org/809333002/diff/20001/src/compiler/code-genera...
> src/compiler/code-generator.cc:270: // No OSR in Turbofan yet...
> Remove this comment.

[titzer, 2015-01-09 14:17:47]

https://codereview.chromium.org/809333002/diff/20001/src/compiler/arm/code-ge...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/809333002/diff/20001/src/compiler/arm/code-ge...
src/compiler/arm/code-generator-arm.cc:890: // TODO(titzer): put an abort into
the header of OSR functions on arm.
On 2014/12/18 11:21:22, Benedikt Meurer wrote:
> MacroAssembler::Abort()

Done.

https://codereview.chromium.org/809333002/diff/20001/src/compiler/code-genera...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/809333002/diff/20001/src/compiler/code-genera...
src/compiler/code-generator.cc:270: // No OSR in Turbofan yet...
On 2014/12/18 11:21:22, Benedikt Meurer wrote:
> Remove this comment.

Done.

[Benedikt Meurer, 2015-01-11 11:50:23]

https://codereview.chromium.org/809333002/diff/130001/src/compiler/instructio...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/809333002/diff/130001/src/compiler/instructio...
src/compiler/instruction-selector.cc:467: // TODO(turbofan): only reverse if > 1
instruction emitted.
std::reverse handles that efficiently. No need to duplicate the work in our
code. Remove this TODO.

https://codereview.chromium.org/809333002/diff/130001/src/compiler/instructio...
src/compiler/instruction-selector.cc:688: return VisitOsrValue(node);
Missing MarkAsReference(node) here.

https://codereview.chromium.org/809333002/diff/130001/src/compiler/osr.h
File src/compiler/osr.h (right):

https://codereview.chromium.org/809333002/diff/130001/src/compiler/osr.h#newc...
src/compiler/osr.h:78: // an outer loop).
Nice explanation!

https://codereview.chromium.org/809333002/diff/130001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/809333002/diff/130001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:863: Run<OsrDeconstructionPhase>();
Hm, can't we make the OSR deconstruction part of the late control reduction?

[Benedikt Meurer, 2015-01-11 11:52:06]

Almost there. Will review the tests in detail tomorrow.

[Benedikt Meurer, 2015-01-12 05:36:27]

https://codereview.chromium.org/809333002/diff/130001/test/cctest/compiler/te...
File test/cctest/compiler/test-osr.cc (right):

https://codereview.chromium.org/809333002/diff/130001/test/cctest/compiler/te...
test/cctest/compiler/test-osr.cc:1: // Copyright 2014 the V8 project authors.
All rights reserved.
Nit: 2015 ;-)

https://codereview.chromium.org/809333002/diff/130001/test/cctest/compiler/te...
test/cctest/compiler/test-osr.cc:36: static Operator
kIntLt(IrOpcode::kInt32LessThan, Operator::kPure,
Why not use the MachineOperatorBuilder instead?

[titzer, 2015-01-12 10:43:54]

https://codereview.chromium.org/809333002/diff/130001/src/compiler/instructio...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/809333002/diff/130001/src/compiler/instructio...
src/compiler/instruction-selector.cc:467: // TODO(turbofan): only reverse if > 1
instruction emitted.
On 2015/01/11 11:50:22, Benedikt Meurer wrote:
> std::reverse handles that efficiently. No need to duplicate the work in our
> code. Remove this TODO.

Hopefully so, TODO removed.

https://codereview.chromium.org/809333002/diff/130001/src/compiler/instructio...
src/compiler/instruction-selector.cc:688: return VisitOsrValue(node);
On 2015/01/11 11:50:22, Benedikt Meurer wrote:
> Missing MarkAsReference(node) here.

Done.

https://codereview.chromium.org/809333002/diff/130001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/809333002/diff/130001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:863: Run<OsrDeconstructionPhase>();
On 2015/01/11 11:50:22, Benedikt Meurer wrote:
> Hm, can't we make the OSR deconstruction part of the late control reduction?

As discussed in person, deconstruction will require a bit of loop peeling before
the control reduction, so it's still probably a good idea to encapsulate it in
OsrHelper.

If we make ControlReducer into a real reducer, it will be easier to maybe make
the Osr deconstruction a reducer that gets mixed in with the late control
reducer.

https://codereview.chromium.org/809333002/diff/130001/test/cctest/compiler/te...
File test/cctest/compiler/test-osr.cc (right):

https://codereview.chromium.org/809333002/diff/130001/test/cctest/compiler/te...
test/cctest/compiler/test-osr.cc:36: static Operator
kIntLt(IrOpcode::kInt32LessThan, Operator::kPure,
On 2015/01/12 05:36:27, Benedikt Meurer wrote:
> Why not use the MachineOperatorBuilder instead?

I think we should add a TestOperatorBuilder, since the actual operator isn't
that important in the test. It was just nice to see when the graph was printed
out. What do think?

[Benedikt Meurer, 2015-01-12 11:12:11]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2015-01-12 11:12:12]

LGTM

https://codereview.chromium.org/809333002/diff/130001/test/cctest/compiler/te...
File test/cctest/compiler/test-osr.cc (right):

https://codereview.chromium.org/809333002/diff/130001/test/cctest/compiler/te...
test/cctest/compiler/test-osr.cc:36: static Operator
kIntLt(IrOpcode::kInt32LessThan, Operator::kPure,
Mhm, yeah we should think about something like that. And probably also remove
the DCHECK from Node::opcode then. But let's do that in a separate CL.

[commit-bot: I haz the power, 2015-01-12 11:12:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/809333002/150001

[commit-bot: I haz the power, 2015-01-12 11:39:56]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2015-01-12 11:40:08]

Patchset 9 (id:??) landed as
https://crrev.com/159b14172f75ddb89e5daf2cceb9078f1f294916
Cr-Commit-Position: refs/heads/master@{#26020}