[turbofan] Allow indexed access to node inputs/input_edges

[turbofan] Allow indexed access to node inputs/input_edges

Node::InputCount() and ::InputAt() have to check for inline/out-of-line
inputs every time they are called. The compiler doesn't seem to be very
good at caching the result of this check, meaning that it (and all its
jumps) would happen for every node access.

Previously we would get around this sometimes, by using Node::inputs(),
which returned a Node::Inputs iterable over node inputs. However,
sometimes node access is more convenient using an index, or we also
want to access the count. This patch adds an index accessor and 'count'
method to Node::Inputs, and replaces several uses of InputCount and
InputAt with this accessor.

Review-Url: https://codereview.chromium.org/2617123002
Cr-Commit-Position: refs/heads/master@{#42179}
Committed: https://chromium.googlesource.com/v8/v8/+/6873f14b60bd2747c0a3a4650fc17a3186c1e217

[Leszek Swirski, 2017-01-06 16:55:45]

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

[commit-bot: I haz the power, 2017-01-06 16:55:49]

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

[Leszek Swirski, 2017-01-06 16:56:11]

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

[commit-bot: I haz the power, 2017-01-06 16:56:19]

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, 2017-01-06 17:22:44]

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

[commit-bot: I haz the power, 2017-01-06 17:22:44]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2017-01-06 17:28:02]

leszeks@chromium.org changed reviewers:
+ bmeurer@chromium.org, jarin@chromium.org

[Leszek Swirski, 2017-01-06 17:28:03]

Hi Jaro and Benedikt,

What do you think of this little cleanup? I don't see much of a performance
improvement (0.5-1%ish on total time by turbo-stats), but I think it's nice and
it definitely produces smaller, better code.

[Leszek Swirski, 2017-01-10 09:46:29]

Ping for review.

[Jarin, 2017-01-10 10:53:39]

Yeah, the compiler cannot really remember the inline/outline state if there is a
function call in between. However, I am not sure it is really worth to do the
changes; especially if the code gets longer or less readable. It would be more
palatable if the CL was line count negative.

Reluctant lgtm.

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:673: }
This looks longer and harder to read. Do we have to do this?

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/branch-eli...
File src/compiler/branch-elimination.cc (right):

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/branch-eli...
src/compiler/branch-elimination.cc:168:
conditions->Merge(*(node_conditions_.Get(*input_it)));
Again, this is longer and harder to read.

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/graph-redu...
File src/compiler/graph-reducer.cc (right):

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/graph-redu...
src/compiler/graph-reducer.cc:123: Node* input = node_inputs[i];
Nice, I like this more than the iterator-heavy stuff.

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/node.h
File src/compiler/node.h (right):

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/node.h#new...
src/compiler/node.h:403: return inputs.count() > 0 && inputs[0] == nullptr;
I actually think the original function will generate pretty much the same code.

[Leszek Swirski, 2017-01-10 12:01:06]

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

[commit-bot: I haz the power, 2017-01-10 12:01:13]

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

[Leszek Swirski, 2017-01-10 12:03:38]

Addressed your comments Jaro, I had a bit of fun analysing the disassembly of an
x64 release build with and without this patch, the results are interesting.
Looks like bitfields compile a bit painfully for us, we might even want to look
into submitting a clang bug for it.

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:673: }
On 2017/01/10 10:53:38, Jarin wrote:
> This looks longer and harder to read. Do we have to do this?

We don't, especially since this code is dying soon. I've reverted it.

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/branch-eli...
File src/compiler/branch-elimination.cc (right):

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/branch-eli...
src/compiler/branch-elimination.cc:168:
conditions->Merge(*(node_conditions_.Get(*input_it)));
On 2017/01/10 10:53:38, Jarin wrote:
> Again, this is longer and harder to read.

This one I'd rather keep, merges can have a lot of inputs, and the new version
is 531 bytes vs. 656 (on x64 release).

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/node.h
File src/compiler/node.h (right):

https://codereview.chromium.org/2617123002/diff/20001/src/compiler/node.h#new...
src/compiler/node.h:403: return inputs.count() > 0 && inputs[0] == nullptr;
On 2017/01/10 10:53:39, Jarin wrote:
> I actually think the original function will generate pretty much the same
code.

You'd be surprised! I checked the disassembled versions of ReduceTop (because of
inlining) with and without this patch, and the dead check changes from 68 to 44
bytes, 20 to 15 instructions, 5 to 4 loads, and 5 to 4 jumps. I think the
bitfield access to the inline count field may be confusing the compiler.

[commit-bot: I haz the power, 2017-01-10 12:27:58]

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

[commit-bot: I haz the power, 2017-01-10 12:27:59]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2017-01-10 13:53:17]

The CQ bit was checked by leszeks@chromium.org

[Leszek Swirski, 2017-01-10 13:53:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2617123002/#ps40001
(title: "Revert AST graph builder change")

[commit-bot: I haz the power, 2017-01-10 13:53:26]

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-10 13:55:03]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484056397023010,
"parent_rev": "754736d26c07454de8eace0b774a1ab13bf2c764", "commit_rev":
"6873f14b60bd2747c0a3a4650fc17a3186c1e217"}

[commit-bot: I haz the power, 2017-01-10 13:55:07]

Description was changed from

==========
[turbofan] Allow indexed access to node inputs/input_edges

Node::InputCount() and ::InputAt() have to check for inline/out-of-line
inputs every time they are called. The compiler doesn't seem to be very
good at caching the result of this check, meaning that it (and all its
jumps) would happen for every node access.

Previously we would get around this sometimes, by using Node::inputs(),
which returned a Node::Inputs iterable over node inputs. However,
sometimes node access is more convenient using an index, or we also
want to access the count. This patch adds an index accessor and 'count'
method to Node::Inputs, and replaces several uses of InputCount and
InputAt with this accessor.
==========

to

==========
[turbofan] Allow indexed access to node inputs/input_edges

Node::InputCount() and ::InputAt() have to check for inline/out-of-line
inputs every time they are called. The compiler doesn't seem to be very
good at caching the result of this check, meaning that it (and all its
jumps) would happen for every node access.

Previously we would get around this sometimes, by using Node::inputs(),
which returned a Node::Inputs iterable over node inputs. However,
sometimes node access is more convenient using an index, or we also
want to access the count. This patch adds an index accessor and 'count'
method to Node::Inputs, and replaces several uses of InputCount and
InputAt with this accessor.

Review-Url: https://codereview.chromium.org/2617123002
Cr-Commit-Position: refs/heads/master@{#42179}
Committed:
https://chromium.googlesource.com/v8/v8/+/6873f14b60bd2747c0a3a4650fc17a3186c...
==========

[commit-bot: I haz the power, 2017-01-10 13:55:09]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/6873f14b60bd2747c0a3a4650fc17a3186c...