[wasm] Trim graph before scheduling.

[wasm] Trim graph before scheduling.

The scheduler expects a trimmed graph, so we have to trim the graph
before scheduling.

R=titzer@chromium.org, bmeurer@chromium.org
TEST=cctest/test-run-wasm/RunWasmCompiled_GraphTrimming

Committed: https://crrev.com/990236825974e683640f9ca56ddd53e3e831a278
Cr-Commit-Position: refs/heads/master@{#40446}

[ahaas, 2016-10-17 07:15:46]

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

[commit-bot: I haz the power, 2016-10-17 07:15:57]

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

[Benedikt Meurer, 2016-10-17 07:18:46]

LGTM once nit addressed.

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

https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:1653: //  Run<LateGraphTrimmingPhase>();
Nit: Remove this dead code then.

[ahaas, 2016-10-17 07:23:20]

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

[commit-bot: I haz the power, 2016-10-17 07:23:27]

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

[ahaas, 2016-10-17 07:23:57]

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

https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:1653: //  Run<LateGraphTrimmingPhase>();
On 2016/10/17 at 07:18:46, Benedikt Meurer wrote:
> Nit: Remove this dead code then.

Done

[commit-bot: I haz the power, 2016-10-17 07:45:56]

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

[commit-bot: I haz the power, 2016-10-17 07:45:57]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/14657)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)

[titzer, 2016-10-17 07:55:13]

On 2016/10/17 07:23:57, ahaas wrote:
> https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc
> File src/compiler/pipeline.cc (right):
> 
>
https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc#ne...
> src/compiler/pipeline.cc:1653: //  Run<LateGraphTrimmingPhase>();
> On 2016/10/17 at 07:18:46, Benedikt Meurer wrote:
> > Nit: Remove this dead code then.
> 
> Done

lgtm

[Benedikt Meurer, 2016-10-17 08:38:14]

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

https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:1653: //  Run<LateGraphTrimmingPhase>();
Thx!

[ahaas, 2016-10-17 12:02:55]

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

[commit-bot: I haz the power, 2016-10-17 12:03:04]

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

[ahaas, 2016-10-17 12:09:53]

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

[commit-bot: I haz the power, 2016-10-17 12:10:04]

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

[ahaas, 2016-10-17 12:13:20]

On 2016/10/17 at 08:38:14, bmeurer wrote:
> https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc
> File src/compiler/pipeline.cc (right):
> 
>
https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc#ne...
> src/compiler/pipeline.cc:1653: //  Run<LateGraphTrimmingPhase>();
> Thx!

PTAL: There was a problem with the instruction selection for Int32PairXXX nodes
on 32 bit platforms. Graph trimming may delete the projection nodes of
Int32PairXXX nodes, which means that we cannot reliably use the projection nodes
for register allocation. I use the following observation to deal with the
problem: As far as I can see, if a projection node is deleted, then it is always
the projection node of the high word. This means that for Int32PairSub,
Int32PairAdd, and Int32PairMul, if the projection node of the high word gets
deleted, we can just emit code for normal 32 bit Add, Sub, and Mul. For shift
instructions, if the projection node of the high word gets deleted, we just
allocate a temp register instead of an output register.

[titzer, 2016-10-17 12:21:14]

On 2016/10/17 12:13:20, ahaas wrote:
> On 2016/10/17 at 08:38:14, bmeurer wrote:
> > https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc
> > File src/compiler/pipeline.cc (right):
> > 
> >
>
https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc#ne...
> > src/compiler/pipeline.cc:1653: //  Run<LateGraphTrimmingPhase>();
> > Thx!
> 
> PTAL: There was a problem with the instruction selection for Int32PairXXX
nodes
> on 32 bit platforms. Graph trimming may delete the projection nodes of
> Int32PairXXX nodes, which means that we cannot reliably use the projection
nodes
> for register allocation. I use the following observation to deal with the
> problem: As far as I can see, if a projection node is deleted, then it is
always
> the projection node of the high word. This means that for Int32PairSub,
> Int32PairAdd, and Int32PairMul, if the projection node of the high word gets
> deleted, we can just emit code for normal 32 bit Add, Sub, and Mul. For shift
> instructions, if the projection node of the high word gets deleted, we just
> allocate a temp register instead of an output register.

The assumption that only the high word gets deleted isn't sound in general. E.g.
if there is a Int32PairAdd flowing into a shift-right by 32 with no other uses,
then only the high projection will be live.

[ahaas, 2016-10-17 12:36:28]

On 2016/10/17 at 12:21:14, titzer wrote:
> On 2016/10/17 12:13:20, ahaas wrote:
> > On 2016/10/17 at 08:38:14, bmeurer wrote:
> > > https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc
> > > File src/compiler/pipeline.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc#ne...
> > > src/compiler/pipeline.cc:1653: //  Run<LateGraphTrimmingPhase>();
> > > Thx!
> > 
> > PTAL: There was a problem with the instruction selection for Int32PairXXX
nodes
> > on 32 bit platforms. Graph trimming may delete the projection nodes of
> > Int32PairXXX nodes, which means that we cannot reliably use the projection
nodes
> > for register allocation. I use the following observation to deal with the
> > problem: As far as I can see, if a projection node is deleted, then it is
always
> > the projection node of the high word. This means that for Int32PairSub,
> > Int32PairAdd, and Int32PairMul, if the projection node of the high word gets
> > deleted, we can just emit code for normal 32 bit Add, Sub, and Mul. For
shift
> > instructions, if the projection node of the high word gets deleted, we just
> > allocate a temp register instead of an output register.
> 
> The assumption that only the high word gets deleted isn't sound in general.
E.g. if there is a Int32PairAdd flowing into a shift-right by 32 with no other
uses, then only the high projection will be live.

That's true, but we haven't implemented that optimization yet.

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

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

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

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-17 12:44:11]

On 2016/10/17 12:36:28, ahaas wrote:
> On 2016/10/17 at 12:21:14, titzer wrote:
> > On 2016/10/17 12:13:20, ahaas wrote:
> > > On 2016/10/17 at 08:38:14, bmeurer wrote:
> > > >
https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc
> > > > File src/compiler/pipeline.cc (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc#ne...
> > > > src/compiler/pipeline.cc:1653: //  Run<LateGraphTrimmingPhase>();
> > > > Thx!
> > > 
> > > PTAL: There was a problem with the instruction selection for Int32PairXXX
> nodes
> > > on 32 bit platforms. Graph trimming may delete the projection nodes of
> > > Int32PairXXX nodes, which means that we cannot reliably use the projection
> nodes
> > > for register allocation. I use the following observation to deal with the
> > > problem: As far as I can see, if a projection node is deleted, then it is
> always
> > > the projection node of the high word. This means that for Int32PairSub,
> > > Int32PairAdd, and Int32PairMul, if the projection node of the high word
gets
> > > deleted, we can just emit code for normal 32 bit Add, Sub, and Mul. For
> shift
> > > instructions, if the projection node of the high word gets deleted, we
just
> > > allocate a temp register instead of an output register.
> > 
> > The assumption that only the high word gets deleted isn't sound in general.
> E.g. if there is a Int32PairAdd flowing into a shift-right by 32 with no other
> uses, then only the high projection will be live.
> 
> That's true, but we haven't implemented that optimization yet.

We will, then we'll have revisit this issue again. I think it's better to just
go ahead and handle the general case.

[ahaas, 2016-10-18 13:46:04]

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

[commit-bot: I haz the power, 2016-10-18 13:46:10]

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

[ahaas, 2016-10-18 13:49:57]

On 2016/10/17 at 12:44:11, titzer wrote:
> On 2016/10/17 12:36:28, ahaas wrote:
> > On 2016/10/17 at 12:21:14, titzer wrote:
> > > On 2016/10/17 12:13:20, ahaas wrote:
> > > > On 2016/10/17 at 08:38:14, bmeurer wrote:
> > > > >
https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc
> > > > > File src/compiler/pipeline.cc (right):
> > > > > 
> > > > >
> > > >
> >
https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc#ne...
> > > > > src/compiler/pipeline.cc:1653: //  Run<LateGraphTrimmingPhase>();
> > > > > Thx!
> > > > 
> > > > PTAL: There was a problem with the instruction selection for
Int32PairXXX
> > nodes
> > > > on 32 bit platforms. Graph trimming may delete the projection nodes of
> > > > Int32PairXXX nodes, which means that we cannot reliably use the
projection
> > nodes
> > > > for register allocation. I use the following observation to deal with
the
> > > > problem: As far as I can see, if a projection node is deleted, then it
is
> > always
> > > > the projection node of the high word. This means that for Int32PairSub,
> > > > Int32PairAdd, and Int32PairMul, if the projection node of the high word
gets
> > > > deleted, we can just emit code for normal 32 bit Add, Sub, and Mul. For
> > shift
> > > > instructions, if the projection node of the high word gets deleted, we
just
> > > > allocate a temp register instead of an output register.
> > > 
> > > The assumption that only the high word gets deleted isn't sound in
general.
> > E.g. if there is a Int32PairAdd flowing into a shift-right by 32 with no
other
> > uses, then only the high projection will be live.
> > 
> > That's true, but we haven't implemented that optimization yet.
> 
> We will, then we'll have revisit this issue again. I think it's better to just
go ahead and handle the general case.

Apparently not having Projection(0) is not a big deal because for register
allocation we use the pair node itself and not Projection(0). This means we
handle the case with no Projection(0) node correctly already.

[commit-bot: I haz the power, 2016-10-18 14:11:04]

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

[commit-bot: I haz the power, 2016-10-18 14:11:05]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-10-19 09:15:43]

On 2016/10/18 at 13:49:57, ahaas wrote:
> On 2016/10/17 at 12:44:11, titzer wrote:
> > On 2016/10/17 12:36:28, ahaas wrote:
> > > On 2016/10/17 at 12:21:14, titzer wrote:
> > > > On 2016/10/17 12:13:20, ahaas wrote:
> > > > > On 2016/10/17 at 08:38:14, bmeurer wrote:
> > > > > >
https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc
> > > > > > File src/compiler/pipeline.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > >
https://codereview.chromium.org/2428443002/diff/1/src/compiler/pipeline.cc#ne...
> > > > > > src/compiler/pipeline.cc:1653: //  Run<LateGraphTrimmingPhase>();
> > > > > > Thx!
> > > > > 
> > > > > PTAL: There was a problem with the instruction selection for
Int32PairXXX
> > > nodes
> > > > > on 32 bit platforms. Graph trimming may delete the projection nodes of
> > > > > Int32PairXXX nodes, which means that we cannot reliably use the
projection
> > > nodes
> > > > > for register allocation. I use the following observation to deal with
the
> > > > > problem: As far as I can see, if a projection node is deleted, then it
is
> > > always
> > > > > the projection node of the high word. This means that for
Int32PairSub,
> > > > > Int32PairAdd, and Int32PairMul, if the projection node of the high
word gets
> > > > > deleted, we can just emit code for normal 32 bit Add, Sub, and Mul.
For
> > > shift
> > > > > instructions, if the projection node of the high word gets deleted, we
just
> > > > > allocate a temp register instead of an output register.
> > > > 
> > > > The assumption that only the high word gets deleted isn't sound in
general.
> > > E.g. if there is a Int32PairAdd flowing into a shift-right by 32 with no
other
> > > uses, then only the high projection will be live.
> > > 
> > > That's true, but we haven't implemented that optimization yet.
> > 
> > We will, then we'll have revisit this issue again. I think it's better to
just go ahead and handle the general case.
> 
> Apparently not having Projection(0) is not a big deal because for register
allocation we use the pair node itself and not Projection(0). This means we
handle the case with no Projection(0) node correctly already.

PTAL

[titzer, 2016-10-19 09:20:43]

https://codereview.chromium.org/2428443002/diff/80001/src/compiler/ia32/instr...
File src/compiler/ia32/instruction-selector-ia32.cc (right):

https://codereview.chromium.org/2428443002/diff/80001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:743: outputs[output_count++] =
g.DefineAsFixed(projection1, edx);
This doesn't seem quite right. Isn't edx overwritten, even if the result is not
used?

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

https://codereview.chromium.org/2428443002/diff/80001/src/compiler/instructio...
src/compiler/instruction-selector.cc:939: MarkAsWord32(projection0);
If projection0 is not found, do you need to mark {node} as word32?

https://codereview.chromium.org/2428443002/diff/80001/test/cctest/compiler/te...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/2428443002/diff/80001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:4164:
TEST(RunInt32PairAddUseOnlyHighWord) {
Can you add tests for all the Int32Pair* operations?

https://codereview.chromium.org/2428443002/diff/80001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-64.cc (right):

https://codereview.chromium.org/2428443002/diff/80001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-64.cc:150: WASM_EXEC_TEST(I64AddUseOnlyLowWord) {
Can you also add tests for using only the high word?

[ahaas, 2016-10-19 09:58:32]

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

[commit-bot: I haz the power, 2016-10-19 09:58:41]

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

[ahaas, 2016-10-19 09:59:19]

https://codereview.chromium.org/2428443002/diff/80001/src/compiler/ia32/instr...
File src/compiler/ia32/instruction-selector-ia32.cc (right):

https://codereview.chromium.org/2428443002/diff/80001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:743: outputs[output_count++] =
g.DefineAsFixed(projection1, edx);
On 2016/10/19 at 09:20:42, titzer wrote:
> This doesn't seem quite right. Isn't edx overwritten, even if the result is
not used?

We always use edx because it is one of the input registers. If projection1
exists, then we also define edx as an output register.

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

https://codereview.chromium.org/2428443002/diff/80001/src/compiler/instructio...
src/compiler/instruction-selector.cc:939: MarkAsWord32(projection0);
On 2016/10/19 at 09:20:42, titzer wrote:
> If projection0 is not found, do you need to mark {node} as word32?

I mark {node} now always as word32.

https://codereview.chromium.org/2428443002/diff/80001/test/cctest/compiler/te...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/2428443002/diff/80001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:4164:
TEST(RunInt32PairAddUseOnlyHighWord) {
On 2016/10/19 at 09:20:42, titzer wrote:
> Can you add tests for all the Int32Pair* operations?

Done.

https://codereview.chromium.org/2428443002/diff/80001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-64.cc (right):

https://codereview.chromium.org/2428443002/diff/80001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-64.cc:150: WASM_EXEC_TEST(I64AddUseOnlyLowWord) {
On 2016/10/19 at 09:20:42, titzer wrote:
> Can you also add tests for using only the high word?

I cannot do that in a WebAssembly test at the moment because there is no
instruction which accesses just the high word. I will do it in
test-run-machops.cc

[titzer, 2016-10-19 11:07:08]

lgtm modulo last comment

https://codereview.chromium.org/2428443002/diff/80001/src/compiler/ia32/instr...
File src/compiler/ia32/instruction-selector-ia32.cc (right):

https://codereview.chromium.org/2428443002/diff/80001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:743: outputs[output_count++] =
g.DefineAsFixed(projection1, edx);
On 2016/10/19 09:59:19, ahaas wrote:
> On 2016/10/19 at 09:20:42, titzer wrote:
> > This doesn't seem quite right. Isn't edx overwritten, even if the result is
> not used?
> 
> We always use edx because it is one of the input registers. If projection1
> exists, then we also define edx as an output register.

That's what I mean. Shouldn't we always mark edx as clobbered so that the
register allocator knows?

[commit-bot: I haz the power, 2016-10-19 11:13:10]

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

[commit-bot: I haz the power, 2016-10-19 11:13:11]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-10-19 15:40:55]

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

[commit-bot: I haz the power, 2016-10-19 15:40:58]

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

[ahaas, 2016-10-19 15:51:53]

On 2016/10/19 at 11:07:08, titzer wrote:
> lgtm modulo last comment
> 
>
https://codereview.chromium.org/2428443002/diff/80001/src/compiler/ia32/instr...
> File src/compiler/ia32/instruction-selector-ia32.cc (right):
> 
>
https://codereview.chromium.org/2428443002/diff/80001/src/compiler/ia32/instr...
> src/compiler/ia32/instruction-selector-ia32.cc:743: outputs[output_count++] =
g.DefineAsFixed(projection1, edx);
> On 2016/10/19 09:59:19, ahaas wrote:
> > On 2016/10/19 at 09:20:42, titzer wrote:
> > > This doesn't seem quite right. Isn't edx overwritten, even if the result
is
> > not used?
> > 
> > We always use edx because it is one of the input registers. If projection1
> > exists, then we also define edx as an output register.
> 
> That's what I mean. Shouldn't we always mark edx as clobbered so that the
register allocator knows?

Done.

[commit-bot: I haz the power, 2016-10-19 16:12:08]

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

[commit-bot: I haz the power, 2016-10-19 16:12:09]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-10-19 16:12:37]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-10-19 16:12:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2428443002/#ps120001
(title: "Use proper temp registers if Projection(1) does not exist.")

[commit-bot: I haz the power, 2016-10-19 16:12:50]

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-10-19 16:21:07]

Committed patchset #7 (id:120001)

[bradn, 2016-10-24 01:06:00]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2016-10-24 01:06:01]

Note this regresses performance for asm->wasm on Fasta, Primes, Box2d,
Corrections, and others by approximately double the runtime:
https://chromeperf.appspot.com/graph_csv?test_path=internal.client.v8/Atom_x6...

Looking into why...

[bradn, 2016-10-24 03:49:43]

Oops, wrong link.

Here's the right one:
https://chromeperf.appspot.com/report?sid=fd3bfa4c0ff033c9fae08631292e2bd681c...

Filling an issue:
https://bugs.chromium.org/p/v8/issues/detail?id=5555

[commit-bot: I haz the power, 2016-11-17 22:07:01]

Description was changed from

==========
[wasm] Trim graph before scheduling.

The scheduler expects a trimmed graph, so we have to trim the graph
before scheduling.

R=titzer@chromium.org, bmeurer@chromium.org
TEST=cctest/test-run-wasm/RunWasmCompiled_GraphTrimming
==========

to

==========
[wasm] Trim graph before scheduling.

The scheduler expects a trimmed graph, so we have to trim the graph
before scheduling.

R=titzer@chromium.org, bmeurer@chromium.org
TEST=cctest/test-run-wasm/RunWasmCompiled_GraphTrimming

Committed: https://crrev.com/990236825974e683640f9ca56ddd53e3e831a278
Cr-Commit-Position: refs/heads/master@{#40446}
==========

[commit-bot: I haz the power, 2016-11-17 22:07:02]

Patchset 7 (id:??) landed as
https://crrev.com/990236825974e683640f9ca56ddd53e3e831a278
Cr-Commit-Position: refs/heads/master@{#40446}