[turbofan] Introduce graph assembler to build effect-control-linearizer sub-graphs.

[turbofan] Introduce graph assembler to build effect-control-linearizer sub-graphs.

Review-Url: https://codereview.chromium.org/2571903004
Cr-Commit-Position: refs/heads/master@{#42013}
Committed: https://chromium.googlesource.com/v8/v8/+/587fda09b72b1b73573860be41978bd5ebdfff2c

[Jarin, 2016-12-19 21:53:35]

The CQ bit was checked by jarin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 21:53:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 21:56:30]

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

[commit-bot: I haz the power, 2016-12-19 21:56:31]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[Jarin, 2016-12-20 12:32:27]

The CQ bit was checked by jarin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 12:32:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 13:01:01]

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

[commit-bot: I haz the power, 2016-12-20 13:01:02]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-12-20 15:16:41]

The CQ bit was checked by jarin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 15:16:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 15:42:25]

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

[commit-bot: I haz the power, 2016-12-20 15:42:25]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-12-22 22:09:51]

The CQ bit was checked by jarin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 22:09:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 22:36:41]

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

[commit-bot: I haz the power, 2016-12-22 22:36:42]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-12-22 22:37:44]

The CQ bit was checked by jarin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 22:37:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 23:02:45]

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

[commit-bot: I haz the power, 2016-12-22 23:02:46]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-12-23 09:03:14]

The CQ bit was checked by jarin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 09:03:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-23 09:22:35]

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

[commit-bot: I haz the power, 2016-12-23 09:22:35]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-12-23 10:26:47]

The CQ bit was checked by jarin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 10:26:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jarin, 2016-12-23 10:29:14]

jarin@chromium.org changed reviewers:
+ ishell@chromium.org

[Jarin, 2016-12-23 10:29:15]

Could you take a look, please?

Perhaps good start is to look at graph-assembler.h.

[commit-bot: I haz the power, 2016-12-23 10:53:42]

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

[commit-bot: I haz the power, 2016-12-23 10:53:43]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-12-27 13:51:06]

Some observations on the current interface.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
File src/compiler/graph-assembler.h (right):

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
src/compiler/graph-assembler.h:185: static GraphAssemblerStaticLabel<MergeCount,
sizeof...(Reps)> MakeLabel(
There is one thing I do not like about this: the MakeLabel factory forces
exposing the copy constructor, but copying labels is dangerous if done on user
side - updates (i.e., gotos and branches) to label copies are basically lost.

I do not have a good idea how to fix this. It would be great if I could disable
the copy constructor, but then the user would have to spell out the variable
count.

As discussed offline, I am also not wild about requiring the explicit merge
count. We could indeed have some mechanism for labels that would store some
fixed numbers of merges on stack. However, I do not like the added complexity
for simple labels (i.e., Label<1, 0>) - they only need one effect and one
control, but the fixed merges solution would require a lot more memory and
book-keeping. I would prefer keep this as is until we come up with a better
solution.

[Igor Sheludko, 2017-01-02 13:23:27]

lgtm. I think it's a good starting point that we can later improve.

nits:

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/effect-co...
File src/compiler/effect-control-linearizer.h (right):

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/effect-co...
src/compiler/effect-control-linearizer.h:123: Node* ChangeInt32ToSmi(Node*
value);
I think these helper methods should live in the GraphAssembler.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/effect-co...
src/compiler/effect-control-linearizer.h:129: Node* SmiShiftBitsConstant();
Same here.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
File src/compiler/graph-assembler.cc (right):

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
src/compiler/graph-assembler.cc:273: Node**
GraphAssemblerLabel::GetBindingsPtrFor(size_t phi_index) {
Please add bounds checks here and in other getters.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
File src/compiler/graph-assembler.h (right):

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
src/compiler/graph-assembler.h:98: bool IsBound() { return is_bound_; }
Please make all getters const.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
src/compiler/graph-assembler.h:107: Node** GetBindingsPtrFor(size_t phi_index) {
Please add bounds checks here and in other getters.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
src/compiler/graph-assembler.h:149: bool IsBound() { return is_bound_; }
Same here.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
src/compiler/graph-assembler.h:185: static GraphAssemblerStaticLabel<MergeCount,
sizeof...(Reps)> MakeLabel(
On 2016/12/27 13:51:06, Jarin wrote:
> There is one thing I do not like about this: the MakeLabel factory forces
> exposing the copy constructor, but copying labels is dangerous if done on user
> side - updates (i.e., gotos and branches) to label copies are basically lost.
> 
> I do not have a good idea how to fix this. It would be great if I could
disable
> the copy constructor, but then the user would have to spell out the variable
> count.
> 
> As discussed offline, I am also not wild about requiring the explicit merge
> count. We could indeed have some mechanism for labels that would store some
> fixed numbers of merges on stack. However, I do not like the added complexity
> for simple labels (i.e., Label<1, 0>) - they only need one effect and one
> control, but the fixed merges solution would require a lot more memory and
> book-keeping. I would prefer keep this as is until we come up with a better
> solution.

I think it's fine for labels to support move semantics, so we can expose only a
move constructor.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
src/compiler/graph-assembler.h:322: label->GetBindingsPtrFor(i)[merged_count] =
var_array[i + 1];
I think it would be better to call here a setter that does bounds checks.

[Jarin, 2017-01-02 15:24:54]

The CQ bit was checked by jarin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-02 15:24:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jarin, 2017-01-02 15:26:44]

The CQ bit was unchecked by jarin@chromium.org

[Jarin, 2017-01-02 15:26:45]

The CQ bit was checked by jarin@chromium.org

[Jarin, 2017-01-02 15:26:45]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2571903004/#ps300001
(title: "Address reviewer comments")

[commit-bot: I haz the power, 2017-01-02 15:26:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-02 15:50:56]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1483370805515410,
"parent_rev": "0663c225f8550ff8329c60a60f71038d9ae7bb74", "commit_rev":
"587fda09b72b1b73573860be41978bd5ebdfff2c"}

[commit-bot: I haz the power, 2017-01-02 15:51:01]

Description was changed from

==========
[turbofan] Introduce graph assembler to build effect-control-linearizer
sub-graphs.
==========

to

==========
[turbofan] Introduce graph assembler to build effect-control-linearizer
sub-graphs.

Review-Url: https://codereview.chromium.org/2571903004
Cr-Commit-Position: refs/heads/master@{#42013}
Committed:
https://chromium.googlesource.com/v8/v8/+/587fda09b72b1b73573860be41978bd5ebd...
==========

[commit-bot: I haz the power, 2017-01-02 15:51:02]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/v8/v8/+/587fda09b72b1b73573860be41978bd5ebd...

[Jarin, 2017-01-03 09:27:55]

Forgot to submit the reply.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/effect-co...
File src/compiler/effect-control-linearizer.h (right):

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/effect-co...
src/compiler/effect-control-linearizer.h:123: Node* ChangeInt32ToSmi(Node*
value);
On 2017/01/02 13:23:26, Igor Sheludko wrote:
> I think these helper methods should live in the GraphAssembler.

Acknowledged.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/effect-co...
src/compiler/effect-control-linearizer.h:129: Node* SmiShiftBitsConstant();
On 2017/01/02 13:23:26, Igor Sheludko wrote:
> Same here.

Acknowledged.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
File src/compiler/graph-assembler.cc (right):

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
src/compiler/graph-assembler.cc:273: Node**
GraphAssemblerLabel::GetBindingsPtrFor(size_t phi_index) {
On 2017/01/02 13:23:27, Igor Sheludko wrote:
> Please add bounds checks here and in other getters.

Done.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
File src/compiler/graph-assembler.h (right):

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
src/compiler/graph-assembler.h:98: bool IsBound() { return is_bound_; }
On 2017/01/02 13:23:27, Igor Sheludko wrote:
> Please make all getters const.

Done.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
src/compiler/graph-assembler.h:107: Node** GetBindingsPtrFor(size_t phi_index) {
On 2017/01/02 13:23:27, Igor Sheludko wrote:
> Please add bounds checks here and in other getters.

Done.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
src/compiler/graph-assembler.h:149: bool IsBound() { return is_bound_; }
On 2017/01/02 13:23:27, Igor Sheludko wrote:
> Same here.

Done.

https://codereview.chromium.org/2571903004/diff/260001/src/compiler/graph-ass...
src/compiler/graph-assembler.h:322: label->GetBindingsPtrFor(i)[merged_count] =
var_array[i + 1];
On 2017/01/02 13:23:27, Igor Sheludko wrote:
> I think it would be better to call here a setter that does bounds checks.

Done.