[turbofan] First version of loop peeling.

[turbofan] First version of loop peeling.

BUG=

Committed: https://crrev.com/3442a5f9e0189bad75da1dc8a30467c3827a8637
Cr-Commit-Position: refs/heads/master@{#26149}

[titzer, 2015-01-15 13:08:59]

titzer@chromium.org changed reviewers:
+ bmeurer@chromium.org, mstarzinger@chromium.org

[Benedikt Meurer, 2015-01-15 13:16:16]

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
File test/unittests/compiler/graph-unittest.h (right):

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:78: 
Move these helpers to loop-peeling-unittest.cc. Also they should not depend on
GraphTest, but rather take a JSGraph.

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:80: struct While {
According to the style guide "structs should be used for passive objects that
carry data, and may have associated constants, but lack any functionality other
than access/setting the data members. The accessing/setting of fields is done by
directly accessing the fields rather than through method invocations. Methods
should not provide behavior but should only be used to set up the data members,
e.g., constructor, destructor, Initialize(), Reset(), Validate().", so this has
to be class then. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._C...

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:108: struct Branch {
See above.

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:123: struct Counter {
See above.

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:129: Counter(While& w,
MachineOperatorBuilder* machine, int32_t b, int32_t k)
According to the google style guide: All parameters passed by reference must be
labeled const.

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:134: Counter(While& w,
MachineOperatorBuilder* machine, Node* b, Node* k)
According to the google style guide: All parameters passed by reference must be
labeled const.

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:139: void Build(While& w,
MachineOperatorBuilder* machine) {
According to the google style guide: All parameters passed by reference must be
labeled const.

[titzer, 2015-01-15 13:36:52]

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
File test/unittests/compiler/graph-unittest.h (right):

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:78: 
On 2015/01/15 13:16:16, Benedikt Meurer wrote:
> Move these helpers to loop-peeling-unittest.cc. Also they should not depend on
> GraphTest, but rather take a JSGraph.

The plan is to use them in other tests, so maybe another header is more
appropriate, but atm I don't think we are suffering from the too-few header
files problem.

And, they don't really need a JSGraph, just a Graph* and a
CommonOperatorBuilder*, which GraphTest has.

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:80: struct While {
On 2015/01/15 13:16:16, Benedikt Meurer wrote:
> According to the style guide "structs should be used for passive objects that
> carry data, and may have associated constants, but lack any functionality
other
> than access/setting the data members. The accessing/setting of fields is done
by
> directly accessing the fields rather than through method invocations. Methods
> should not provide behavior but should only be used to set up the data
members,
> e.g., constructor, destructor, Initialize(), Reset(), Validate().", so this
has
> to be class then. See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._C...

I'd prefer to keep these as structs to avoid accessors for all the members, so
that means what? Moving Chain() and Nest() and Phi() to somewhere else?

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:129: Counter(While& w,
MachineOperatorBuilder* machine, int32_t b, int32_t k)
On 2015/01/15 13:16:16, Benedikt Meurer wrote:
> According to the google style guide: All parameters passed by reference must
be
> labeled const.

Done.

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:134: Counter(While& w,
MachineOperatorBuilder* machine, Node* b, Node* k)
On 2015/01/15 13:16:16, Benedikt Meurer wrote:
> According to the google style guide: All parameters passed by reference must
be
> labeled const.

Done.

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:139: void Build(While& w,
MachineOperatorBuilder* machine) {
On 2015/01/15 13:16:16, Benedikt Meurer wrote:
> According to the google style guide: All parameters passed by reference must
be
> labeled const.

Done.

[Benedikt Meurer, 2015-01-15 13:43:10]

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
File test/unittests/compiler/graph-unittest.h (right):

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:78: 
Well this kind of goes against the purpose of GraphTest. GraphTest should be a
base class for Graph tests and as independent and generic as possible. So please
move these to loop-peeling-unittest.cc and only hoist them into a header when
really necessary. And if you only need a Graph and CommonOperatorBuilder, then
pass them as parameters. GraphTest should not be used as a way to pass around
Graph+CommonOperatorBuilder.

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:80: struct While {
Personally I don't like these helper structs at all, esp. not in header files.
IMHO it's easier to read the code without these helpers, even if the code is
twice as long, but then you don't need to lookup/remember the helper magic. The
Diamond class already caused quite some trouble.

But I'm fine with these as long as they are local within the .cc file, ideally
within the class that they are built for.

[titzer, 2015-01-16 16:15:37]

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
File test/unittests/compiler/graph-unittest.h (right):

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:78: 
On 2015/01/15 13:43:09, Benedikt Meurer wrote:
> Well this kind of goes against the purpose of GraphTest. GraphTest should be a
> base class for Graph tests and as independent and generic as possible. So
please
> move these to loop-peeling-unittest.cc and only hoist them into a header when
> really necessary. And if you only need a Graph and CommonOperatorBuilder, then
> pass them as parameters. GraphTest should not be used as a way to pass around
> Graph+CommonOperatorBuilder.

PTAL, I've made these utilities into standalone structs and added helper methods
to the LoopPeelerTest to create and chain them together.

[Benedikt Meurer, 2015-01-16 17:55:37]

Thanks. Next bunch of comments.

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
File test/unittests/compiler/graph-unittest.h (right):

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:78: 
Thanks. Now you can undo the changes to graph-unittest.h as they're no longer
needed.

https://codereview.chromium.org/816053002/diff/100001/src/compiler/common-ope...
File src/compiler/common-operator.h (right):

https://codereview.chromium.org/816053002/diff/100001/src/compiler/common-ope...
src/compiler/common-operator.h:59: const Operator* ResizeMergeOperator(const
Operator* const);
Dead method?

https://codereview.chromium.org/816053002/diff/100001/src/compiler/common-ope...
src/compiler/common-operator.h:206: const Operator* ResizeMergeOrPhi(const
Operator* op, int size);
Can you move this into loop-peeling.cc? I don't think there will be any uses
outside of that, and it's not really something that the builder should be
concerned with.

https://codereview.chromium.org/816053002/diff/100001/src/compiler/loop-analy...
File src/compiler/loop-analysis.cc (right):

https://codereview.chromium.org/816053002/diff/100001/src/compiler/loop-analy...
src/compiler/loop-analysis.cc:1: // Copyright 2014 the V8 project authors. All
rights reserved.
2015 :-)

https://codereview.chromium.org/816053002/diff/100001/src/compiler/loop-peeli...
File src/compiler/loop-peeling.cc (right):

https://codereview.chromium.org/816053002/diff/100001/src/compiler/loop-peeli...
src/compiler/loop-peeling.cc:69: PeeledIterationImpl* impl =
static_cast<PeeledIterationImpl*>(this);
How about avoiding the PeeledIterationImpl and just put the NodeVector into the
PeeledIteration class itself?

https://codereview.chromium.org/816053002/diff/100001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/816053002/diff/100001/src/flag-definitions.h#...
src/flag-definitions.h:417: DEFINE_BOOL(stress_loop_peeling, false, "stress loop
peeling optimization")
I'd prefer if all turbofan related flags have "turbo" in their name. Makes it
easier to spot them.

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
File test/unittests/compiler/graph-unittest.h (left):

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
test/unittests/compiler/graph-unittest.h:34: protected:
Reinsert the protected.

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
test/unittests/compiler/graph-unittest.h:67: protected:
Reinsert the protected.

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
File test/unittests/compiler/graph-unittest.h (right):

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
test/unittests/compiler/graph-unittest.h:10: #include
"src/compiler/machine-operator.h"
Remove these two includes again.

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
File test/unittests/compiler/loop-peeling-unittest.cc (right):

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
test/unittests/compiler/loop-peeling-unittest.cc:9: #include
"src/compiler/js-operator.h"
This include seems unnecessary.

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
test/unittests/compiler/loop-peeling-unittest.cc:10: #include
"src/compiler/js-typed-lowering.h"
This include seems unnecessary.

[titzer, 2015-01-19 09:47:12]

Ascii art in the making....

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
File test/unittests/compiler/graph-unittest.h (right):

https://codereview.chromium.org/816053002/diff/40001/test/unittests/compiler/...
test/unittests/compiler/graph-unittest.h:78: 
On 2015/01/16 17:55:36, Benedikt Meurer wrote:
> Thanks. Now you can undo the changes to graph-unittest.h as they're no longer
> needed.

Done.

https://codereview.chromium.org/816053002/diff/100001/src/compiler/common-ope...
File src/compiler/common-operator.h (right):

https://codereview.chromium.org/816053002/diff/100001/src/compiler/common-ope...
src/compiler/common-operator.h:59: const Operator* ResizeMergeOperator(const
Operator* const);
On 2015/01/16 17:55:36, Benedikt Meurer wrote:
> Dead method?

Done.

https://codereview.chromium.org/816053002/diff/100001/src/compiler/common-ope...
src/compiler/common-operator.h:206: const Operator* ResizeMergeOrPhi(const
Operator* op, int size);
On 2015/01/16 17:55:36, Benedikt Meurer wrote:
> Can you move this into loop-peeling.cc? I don't think there will be any uses
> outside of that, and it's not really something that the builder should be
> concerned with.

I want to convert the control reducer and the graph builder to use it sometime
in the near future.

https://codereview.chromium.org/816053002/diff/100001/src/compiler/loop-analy...
File src/compiler/loop-analysis.cc (right):

https://codereview.chromium.org/816053002/diff/100001/src/compiler/loop-analy...
src/compiler/loop-analysis.cc:1: // Copyright 2014 the V8 project authors. All
rights reserved.
On 2015/01/16 17:55:36, Benedikt Meurer wrote:
> 2015 :-)

First committed last month...

https://codereview.chromium.org/816053002/diff/100001/src/compiler/loop-peeli...
File src/compiler/loop-peeling.cc (right):

https://codereview.chromium.org/816053002/diff/100001/src/compiler/loop-peeli...
src/compiler/loop-peeling.cc:69: PeeledIterationImpl* impl =
static_cast<PeeledIterationImpl*>(this);
On 2015/01/16 17:55:36, Benedikt Meurer wrote:
> How about avoiding the PeeledIterationImpl and just put the NodeVector into
the
> PeeledIteration class itself?

Was hoping to hide the implementation for now, since it is a linear search at
the moment and it's only used in testing. Still not sure if we need to have the
peeled iteration exposed in production code.

https://codereview.chromium.org/816053002/diff/100001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/816053002/diff/100001/src/flag-definitions.h#...
src/flag-definitions.h:417: DEFINE_BOOL(stress_loop_peeling, false, "stress loop
peeling optimization")
On 2015/01/16 17:55:36, Benedikt Meurer wrote:
> I'd prefer if all turbofan related flags have "turbo" in their name. Makes it
> easier to spot them.

Done.

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
File test/unittests/compiler/graph-unittest.h (left):

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
test/unittests/compiler/graph-unittest.h:34: protected:
On 2015/01/16 17:55:36, Benedikt Meurer wrote:
> Reinsert the protected.

Done.

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
test/unittests/compiler/graph-unittest.h:67: protected:
On 2015/01/16 17:55:36, Benedikt Meurer wrote:
> Reinsert the protected.

Done.

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
File test/unittests/compiler/graph-unittest.h (right):

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
test/unittests/compiler/graph-unittest.h:10: #include
"src/compiler/machine-operator.h"
On 2015/01/16 17:55:36, Benedikt Meurer wrote:
> Remove these two includes again.

Done.

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
File test/unittests/compiler/loop-peeling-unittest.cc (right):

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
test/unittests/compiler/loop-peeling-unittest.cc:9: #include
"src/compiler/js-operator.h"
On 2015/01/16 17:55:36, Benedikt Meurer wrote:
> This include seems unnecessary.

Done.

https://codereview.chromium.org/816053002/diff/100001/test/unittests/compiler...
test/unittests/compiler/loop-peeling-unittest.cc:10: #include
"src/compiler/js-typed-lowering.h"
On 2015/01/16 17:55:36, Benedikt Meurer wrote:
> This include seems unnecessary.

Done.

[Benedikt Meurer, 2015-01-19 17:47:28]

LGTM with CommonOperatorBuilder::ResizeMergeOrPhi() concern remaining.

https://codereview.chromium.org/816053002/diff/100001/src/compiler/common-ope...
File src/compiler/common-operator.h (right):

https://codereview.chromium.org/816053002/diff/100001/src/compiler/common-ope...
src/compiler/common-operator.h:206: const Operator* ResizeMergeOrPhi(const
Operator* op, int size);
That may introduce another use for that, in which case we can hoist it into the
builder. But please no premature generalization at this point, so please move it
to loop-peeling.cc for now.

[Benedikt Meurer, 2015-01-19 17:47:43]

Love the ASCII art BTW!

[titzer, 2015-01-20 08:31:23]

On 2015/01/19 17:47:28, Benedikt Meurer wrote:
> LGTM with CommonOperatorBuilder::ResizeMergeOrPhi() concern remaining.
> 
>
https://codereview.chromium.org/816053002/diff/100001/src/compiler/common-ope...
> File src/compiler/common-operator.h (right):
> 
>
https://codereview.chromium.org/816053002/diff/100001/src/compiler/common-ope...
> src/compiler/common-operator.h:206: const Operator* ResizeMergeOrPhi(const
> Operator* op, int size);
> That may introduce another use for that, in which case we can hoist it into
the
> builder. But please no premature generalization at this point, so please move
it
> to loop-peeling.cc for now.

Done.

[Benedikt Meurer, 2015-01-20 08:31:36]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2015-01-20 08:31:37]

lgtm

[commit-bot: I haz the power, 2015-01-20 08:31:47]

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

[commit-bot: I haz the power, 2015-01-20 08:36:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-20 08:36:29]

Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
  v8_mac_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/2364)

[Benedikt Meurer, 2015-01-20 08:37:01]

https://codereview.chromium.org/816053002/diff/180001/src/compiler/common-ope...
File src/compiler/common-operator.h (right):

https://codereview.chromium.org/816053002/diff/180001/src/compiler/common-ope...
src/compiler/common-operator.h:203: const Operator* ResizeMergeOrPhi(const
Operator* op, int size);
BTW: You can revert changes to common-operator.h now.

[titzer, 2015-01-20 08:56:36]

On 2015/01/20 08:37:01, Benedikt Meurer wrote:
>
https://codereview.chromium.org/816053002/diff/180001/src/compiler/common-ope...
> File src/compiler/common-operator.h (right):
> 
>
https://codereview.chromium.org/816053002/diff/180001/src/compiler/common-ope...
> src/compiler/common-operator.h:203: const Operator* ResizeMergeOrPhi(const
> Operator* op, int size);
> BTW: You can revert changes to common-operator.h now.

Done.

[titzer, 2015-01-20 08:56:58]

The CQ bit was checked by titzer@chromium.org

[commit-bot: I haz the power, 2015-01-20 08:57:08]

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

[commit-bot: I haz the power, 2015-01-20 09:13:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-20 09:13:10]

Try jobs failed on following builders:
  v8_win64_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel/builds/1268)

[titzer, 2015-01-20 09:17:04]

The CQ bit was checked by titzer@chromium.org

[commit-bot: I haz the power, 2015-01-20 09:17:18]

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

[commit-bot: I haz the power, 2015-01-20 09:45:11]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-01-20 09:45:29]

Patchset 12 (id:??) landed as
https://crrev.com/3442a5f9e0189bad75da1dc8a30467c3827a8637
Cr-Commit-Position: refs/heads/master@{#26149}