[turbofan] Sparse representation for state values

[turbofan] Sparse representation for state values

Add a more efficient encoding for state values that have a large number of
optimized-out inputs.

Review-Url: https://codereview.chromium.org/2509623002
Cr-Commit-Position: refs/heads/master@{#42088}
Committed: https://chromium.googlesource.com/v8/v8/+/68f1a37f8e34e26963f768a8c6129dde9a02c2ad

[Leszek Swirski, 2016-11-16 09:29:50]

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

[Leszek Swirski, 2016-11-16 09:29:50]

Hey Jaro and Benedikt,

This is my WIP for the state value sparse encoding, let me know what you think.

[Leszek Swirski, 2016-11-16 13:41:51]

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

[commit-bot: I haz the power, 2016-11-16 13:41:53]

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-11-16 13:45:22]

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

[commit-bot: I haz the power, 2016-11-16 13:45:23]

Dry run: Try jobs failed on following builders:
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
  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-11-16 15:32:43]

Overall, this feels really hacky - is there any measurable improvement?

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/common-ope...
File src/compiler/common-operator.cc (right):

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/common-ope...
src/compiler/common-operator.cc:252: .first;
This is confusing - where is 'types' used?

Should not it be returned here?

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

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/instructio...
src/compiler/instruction-selector.cc:489:
inputs->push_back(g->TempImmediate(FrameStateDescriptor::kImpossibleValue));
This should be the optimized_out value, no?

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/instructio...
src/compiler/instruction-selector.cc:491: StateValueDescriptor::Plain(zone,
MachineType::None()));
MachineType::AnyTagged()

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/state-valu...
File src/compiler/state-values-utils.cc (right):

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/state-valu...
src/compiler/state-values-utils.cc:67: } else if (node->opcode() ==
IrOpcode::kTypedStateValues) {
Does the TypedStateValues actually ever get here?

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/state-valu...
src/compiler/state-values-utils.cc:235: height++;
I am guesisng this will get the tree height wrong because it does not take the
optimized_out constants into account. No?

[Leszek Swirski, 2016-11-17 09:11:58]

Description was changed from

==========
[turbofan] Sparse represantation for state values

Add a more efficient encoding for state values that have a large number of
optimized-out inputs.
==========

to

==========
[turbofan] Sparse representation for state values

Add a more efficient encoding for state values that have a large number of
optimized-out inputs.
==========

[Leszek Swirski, 2016-11-17 09:23:05]

Comments commented upon.

Sadly, this doesn't actually seem to have a performance improvement, and in fact
has a performance impact on Mandreel and zlib. I find this kind of surprising, I
still think this might be an avenue worth pursuing, but it may need some more
in-depth analysis.

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/common-ope...
File src/compiler/common-operator.cc (right):

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/common-ope...
src/compiler/common-operator.cc:252: .first;
On 2016/11/16 15:32:42, Jarin wrote:
> This is confusing - where is 'types' used?
> 
> Should not it be returned here?

Whoops, yes it should. Bad rebase. I think the below return happened to still
work because const ZoneVector<MachineType>* is the first field of the pair
anyway, but yeah, this should be a return.

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

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/instructio...
src/compiler/instruction-selector.cc:489:
inputs->push_back(g->TempImmediate(FrameStateDescriptor::kImpossibleValue));
On 2016/11/16 15:32:42, Jarin wrote:
> This should be the optimized_out value, no?

Yeah, I had a problem here that the jsgraph isn't available, so I couldn't get
at that node. I figured that any value here would be sufficiently valid (if not
a great marker) for testing, so that I don't have to wire the jsgraph through
into here.

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/instructio...
src/compiler/instruction-selector.cc:491: StateValueDescriptor::Plain(zone,
MachineType::None()));
On 2016/11/16 15:32:42, Jarin wrote:
> MachineType::AnyTagged()

As above.

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/state-valu...
File src/compiler/state-values-utils.cc (right):

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/state-valu...
src/compiler/state-values-utils.cc:67: } else if (node->opcode() ==
IrOpcode::kTypedStateValues) {
On 2016/11/16 15:32:42, Jarin wrote:
> Does the TypedStateValues actually ever get here?

Good point, probably not.

https://codereview.chromium.org/2509623002/diff/20001/src/compiler/state-valu...
src/compiler/state-values-utils.cc:235: height++;
On 2016/11/16 15:32:43, Jarin wrote:
> I am guesisng this will get the tree height wrong because it does not take the
> optimized_out constants into account. No?

Yes, this was my guess too. However, I think that this doesn't really matter,
because over-estimating the height of the tree will only over-allocate the
working space; the actual trees generated should (I think) be collapsed if they
are too tall by the if(count == 1) check in BuildTree.

[Leszek Swirski, 2016-12-05 12:31:19]

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

[commit-bot: I haz the power, 2016-12-05 12:31:23]

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-05 12:34:34]

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

[commit-bot: I haz the power, 2016-12-05 12:34:35]

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

[Leszek Swirski, 2016-12-05 13:24:19]

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

[commit-bot: I haz the power, 2016-12-05 13:24:30]

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-05 13:50:08]

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

[commit-bot: I haz the power, 2016-12-05 13:50:08]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2016-12-06 09:42:32]

Ping for another review (now that there are performance improvements).

[Jarin, 2016-12-06 10:35:16]

On 2016/12/06 09:42:32, Leszek Swirski wrote:
> Ping for another review (now that there are performance improvements).

Sorry for taking so long, we have the PhD summit here. I should be able to look
at this later today.

What improvements do you see?

Also note that you might have to rebase - I did some changes to the instructions
selector data structures.

[Leszek Swirski, 2016-12-06 10:58:35]

On 2016/12/06 10:35:16, Jarin wrote:
> On 2016/12/06 09:42:32, Leszek Swirski wrote:
> > Ping for another review (now that there are performance improvements).
> 
> Sorry for taking so long, we have the PhD summit here. I should be able to
look
> at this later today.

Ah yeah, forgot about that, sorry. No rush in that case.

> What improvements do you see?

I see some (albeit small) improvements on Mandreel and some Sunspider benchmarks
-- I added some graphs to the compile time doc.

> Also note that you might have to rebase - I did some changes to the
instructions
> selector data structures.

Already done, in PS5 (where I also updated the code generator to use
PushOptimizedOut, which is awesome btw).

[Jarin, 2016-12-07 08:56:20]

First batch of comments. (src/compiler/state-values-utils.* will be done
separately.)

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/common-op...
File src/compiler/common-operator.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/common-op...
src/compiler/common-operator.cc:189: return os << p.machine_types() << "|" <<
p.mask();
Please update the printing to print something human readable - the mask will
look like a random number.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/common-op...
src/compiler/common-operator.cc:613: struct StateValuesOperator final : public
Operator1<uint32_t> {
To get a good printing, it would be better to introduce a struct for the mask
and then define << for the struct (unfortunately, you will also have to do
hash_value, == and !=).

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

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/common-op...
src/compiler/common-operator.h:174: uint32_t mask_;
This is a poor name - could you come with a name that describes what the mask
means?

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/instructi...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/instructi...
src/compiler/instruction-selector.cc:517: if (input->opcode() ==
IrOpcode::kHeapConstant) {
Maybe we can remove this ugly dance will now?

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/js-inlini...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/js-inlini...
src/compiler/js-inlining.cc:234: const Operator* op0 = common()->StateValues(0,
0u);
Maybe it would be better if there was some constant for this 0u because it is
not obvious at all what this 0u stands for. WDYT?

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/liveness-...
File src/compiler/liveness-analyzer.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/liveness-...
src/compiler/liveness-analyzer.cc:153: &replacement_node_, 1, nullptr);
How about adding a default value (nullptr) for the liveness? This nullptr here
is a bit confusing.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/simplifie...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/simplifie...
src/compiler/simplified-lowering.cc:1013: uint32_t mask =
OpParameter<uint32_t>(node);
Descriptive name, please.

You should not use OpParameter directly, please, define an accessor in
common-operator.*.

OpParamater is very dangerous because if you decide to change the parameter type
(in CommonOperatorBuilder::StateValues), compiler will not complain about type
mismatch and you will start seeing random "mask". We already had serious bugs
here.

[Jarin (Google), 2016-12-07 15:16:43]

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

[Jarin (Google), 2016-12-07 15:16:44]

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
File src/compiler/state-values-utils.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:22: //     whether live or dead (and is not
representative of a live node)
If really want to use such a complex encoding scheme, there should be a check in
StateValues constructor making sure that the number of live inputs corresponds
to the input count.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:67: uint32_t node_mask =
OpParameter<uint32_t>(node);
No OpParameter, please!

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:160: (!use_mask || virtual_input_count < 31))
{
Magic constants (31) defined at the top of the function, please.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:166: use_mask = true;
What improvement do you see from special casing the all-live case? Is it worth
the extra complexity?

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:187: // mask.
Maybe the remove the comment, the complexity budget on this function is
exhausted.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:200:
liveness->Contains(static_cast<int>(idx))) {
This looks very similar to the code above, perhaps it should be factored out?

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:300: uint32_t GetStateValueMask(Node* node) {
This should live somewhere in common-operator.h

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:351: if (mask == 0 || (mask & 0x1) == 1) {
Nit: Why 0x1? On line 446, you say mask & 1. Perhaps you could do the same here
(and below).

Or actually, I see the pattern "mask != 0 && (mask & 0x1) == 0" and "mask == 0
|| (mask & 0x1) == 1" many times here, perhaps factor into a helper function?

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:364: // We are on a valid (dead) node.
dead -> optimized_out here and elsewhere

[Jarin, 2016-12-08 07:59:43]

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
File src/compiler/state-values-utils.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:15: // sparse; otherwise, they should be
interpreted as follows:
This explanation should probably be in common-operator.h - somewhere close to
StateValues and TypedStateValues.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
File src/compiler/state-values-utils.h (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.h:100: uint32_t mask;
This needs some explanation of what it means.

[Leszek Swirski, 2016-12-08 15:31:53]

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

[commit-bot: I haz the power, 2016-12-08 15:32:05]

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

[Leszek Swirski, 2016-12-08 15:44:31]

Comments addressed (some answered inline), please take another look.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/common-op...
File src/compiler/common-operator.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/common-op...
src/compiler/common-operator.cc:189: return os << p.machine_types() << "|" <<
p.mask();
On 2016/12/07 08:56:20, Jarin wrote:
> Please update the printing to print something human readable - the mask will
> look like a random number.

Done.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/common-op...
src/compiler/common-operator.cc:613: struct StateValuesOperator final : public
Operator1<uint32_t> {
On 2016/12/07 08:56:19, Jarin wrote:
> To get a good printing, it would be better to introduce a struct for the mask
> and then define << for the struct (unfortunately, you will also have to do
> hash_value, == and !=).

Done.

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

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/common-op...
src/compiler/common-operator.h:174: uint32_t mask_;
On 2016/12/07 08:56:20, Jarin wrote:
> This is a poor name - could you come with a name that describes what the mask
> means?

Hm, the only thing I could think of is to expand it to its new type name,
sparse_input_mask. WDYT?

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/instructi...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/instructi...
src/compiler/instruction-selector.cc:517: if (input->opcode() ==
IrOpcode::kHeapConstant) {
On 2016/12/07 08:56:20, Jarin wrote:
> Maybe we can remove this ugly dance will now?

Not yet, I don't think, there still could be optimized_out nodes coming from
AstGraphBuilder.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/js-inlini...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/js-inlini...
src/compiler/js-inlining.cc:234: const Operator* op0 = common()->StateValues(0,
0u);
On 2016/12/07 08:56:20, Jarin wrote:
> Maybe it would be better if there was some constant for this 0u because it is
> not obvious at all what this 0u stands for. WDYT?

Done, replaced with a static factory function (it could be a constant, doesn't
really matter IMO).

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/liveness-...
File src/compiler/liveness-analyzer.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/liveness-...
src/compiler/liveness-analyzer.cc:153: &replacement_node_, 1, nullptr);
On 2016/12/07 08:56:20, Jarin wrote:
> How about adding a default value (nullptr) for the liveness? This nullptr here
> is a bit confusing.

Ah, I had that originally but forgot to add it back in after some refactoring.
Done.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/simplifie...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/simplifie...
src/compiler/simplified-lowering.cc:1013: uint32_t mask =
OpParameter<uint32_t>(node);
On 2016/12/07 08:56:20, Jarin wrote:
> Descriptive name, please.
> 
> You should not use OpParameter directly, please, define an accessor in
> common-operator.*.
> 
> OpParamater is very dangerous because if you decide to change the parameter
type
> (in CommonOperatorBuilder::StateValues), compiler will not complain about type
> mismatch and you will start seeing random "mask". We already had serious bugs
> here.

Done, throughout. Interesting point about the type mismatch, I'm not sure I see
how the accessors in common-operator (which are just wrappers around
OpParameter) help with this?

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
File src/compiler/state-values-utils.cc (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:15: // sparse; otherwise, they should be
interpreted as follows:
On 2016/12/08 07:59:43, Jarin wrote:
> This explanation should probably be in common-operator.h - somewhere close to
> StateValues and TypedStateValues.

Done.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:22: //     whether live or dead (and is not
representative of a live node)
On 2016/12/07 15:16:44, Jarin (Google) wrote:
> If really want to use such a complex encoding scheme, there should be a check
in
> StateValues constructor making sure that the number of live inputs corresponds
> to the input count.

Done.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:67: uint32_t node_mask =
OpParameter<uint32_t>(node);
On 2016/12/07 15:16:44, Jarin (Google) wrote:
> No OpParameter, please!

Done.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:160: (!use_mask || virtual_input_count < 31))
{
On 2016/12/07 15:16:44, Jarin (Google) wrote:
> Magic constants (31) defined at the top of the function, please.

Done (moved into the new sparse input mask type).

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:166: use_mask = true;
On 2016/12/07 15:16:44, Jarin (Google) wrote:
> What improvement do you see from special casing the all-live case? Is it worth
> the extra complexity?

I haven't measured it, but I've removed it since the iterator still has to make
the same checks regardless so I can't imagine it giving a performance benefit.

I had to keep nodes with (only) subtrees dense, so that I wouldn't have to
fiddle with setting the guard bit conditionally on whether the "dump remaining
nodes" path was taken. Conveniently, this simplified the single-input elision to
only trigger if we have a single StateValue input, and we can get rid of the
wrapper check in GetNodeForValues.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:187: // mask.
On 2016/12/07 15:16:44, Jarin (Google) wrote:
> Maybe the remove the comment, the complexity budget on this function is
> exhausted.

Kept the comment (shortened it a bit), after refactoring to simplify this
function.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:200:
liveness->Contains(static_cast<int>(idx))) {
On 2016/12/07 15:16:44, Jarin (Google) wrote:
> This looks very similar to the code above, perhaps it should be factored out?

Done, I had a TODO for it at some point anyway.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:300: uint32_t GetStateValueMask(Node* node) {
On 2016/12/07 15:16:44, Jarin (Google) wrote:
> This should live somewhere in common-operator.h

Done.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:351: if (mask == 0 || (mask & 0x1) == 1) {
On 2016/12/07 15:16:44, Jarin (Google) wrote:
> Nit: Why 0x1? On line 446, you say mask & 1. Perhaps you could do the same
here
> (and below).
> 
> Or actually, I see the pattern "mask != 0 && (mask & 0x1) == 0" and "mask == 0
> || (mask & 0x1) == 1" many times here, perhaps factor into a helper function?

Completely refactored out into sparse input iterator.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.cc:364: // We are on a valid (dead) node.
On 2016/12/07 15:16:44, Jarin (Google) wrote:
> dead -> optimized_out here and elsewhere

Done.

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
File src/compiler/state-values-utils.h (right):

https://codereview.chromium.org/2509623002/diff/100001/src/compiler/state-val...
src/compiler/state-values-utils.h:100: uint32_t mask;
On 2016/12/08 07:59:43, Jarin wrote:
> This needs some explanation of what it means.

Removed after iterator refactor.

[commit-bot: I haz the power, 2016-12-08 15:59:15]

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

[commit-bot: I haz the power, 2016-12-08 15:59:16]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-12-10 10:15:06]

https://codereview.chromium.org/2509623002/diff/120001/src/compiler/common-op...
File src/compiler/common-operator.h (right):

https://codereview.chromium.org/2509623002/diff/120001/src/compiler/common-op...
src/compiler/common-operator.h:178: // So, for N 1s in the bitmask, there are N
- 1 inputs into the node.
Perhaps move lines 166-178 to the actual mask field (line 249) so that people
only read the text when interested in internals of the class.

https://codereview.chromium.org/2509623002/diff/120001/src/compiler/state-val...
File src/compiler/state-values-utils.cc (right):

https://codereview.chromium.org/2509623002/diff/120001/src/compiler/state-val...
src/compiler/state-values-utils.cc:139: WorkingBuffer& input_buffer, size_t&
input_count, size_t& idx,
The arguments to this function are really confusing, esp. for idx and count, it
is not clear what array they refer to. Perhaps rename to values_idx,
values_count?

It would be also nice to invent new names for input_buffer and input_count
because they are in fact output values of the function. Or at least you might
consider returning the mask and input_count in a little struct.

https://codereview.chromium.org/2509623002/diff/120001/src/compiler/state-val...
File src/compiler/state-values-utils.h (right):

https://codereview.chromium.org/2509623002/diff/120001/src/compiler/state-val...
src/compiler/state-values-utils.h:61: const BitVector* liveness);
I think the style guide only allows passing const things by reference.

Input/output parameters should be passed by pointer.

[Leszek Swirski, 2016-12-13 14:33:50]

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

[commit-bot: I haz the power, 2016-12-13 14:33:59]

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

[Leszek Swirski, 2016-12-13 14:35:16]

https://codereview.chromium.org/2509623002/diff/120001/src/compiler/common-op...
File src/compiler/common-operator.h (right):

https://codereview.chromium.org/2509623002/diff/120001/src/compiler/common-op...
src/compiler/common-operator.h:178: // So, for N 1s in the bitmask, there are N
- 1 inputs into the node.
On 2016/12/10 10:15:06, Jarin wrote:
> Perhaps move lines 166-178 to the actual mask field (line 249) so that people
> only read the text when interested in internals of the class.

Good idea, done.

https://codereview.chromium.org/2509623002/diff/120001/src/compiler/state-val...
File src/compiler/state-values-utils.cc (right):

https://codereview.chromium.org/2509623002/diff/120001/src/compiler/state-val...
src/compiler/state-values-utils.cc:139: WorkingBuffer& input_buffer, size_t&
input_count, size_t& idx,
On 2016/12/10 10:15:06, Jarin wrote:
> The arguments to this function are really confusing, esp. for idx and count,
it
> is not clear what array they refer to. Perhaps rename to values_idx,
> values_count?
> 
> It would be also nice to invent new names for input_buffer and input_count
> because they are in fact output values of the function. Or at least you might
> consider returning the mask and input_count in a little struct.

Yeah, this is tough because of the difference between "Node inputs" and
"function inputs". I've renamed to prefix with "node_" instead, WDYT?

https://codereview.chromium.org/2509623002/diff/120001/src/compiler/state-val...
File src/compiler/state-values-utils.h (right):

https://codereview.chromium.org/2509623002/diff/120001/src/compiler/state-val...
src/compiler/state-values-utils.h:61: const BitVector* liveness);
On 2016/12/10 10:15:06, Jarin wrote:
> I think the style guide only allows passing const things by reference.
> 
> Input/output parameters should be passed by pointer.

Updated, thanks.

[commit-bot: I haz the power, 2016-12-13 14:59:11]

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

[commit-bot: I haz the power, 2016-12-13 14:59:12]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2017-01-04 10:02:14]

Ping for review

[Jarin, 2017-01-05 09:02:53]

lgtm

[Leszek Swirski, 2017-01-05 10:10:53]

The CQ bit was checked by leszeks@chromium.org

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

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-05 10:44:45]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483611053098470,
"parent_rev": "8bb2501e7f8d2575d02e3a7e2fab8c431045b838", "commit_rev":
"68f1a37f8e34e26963f768a8c6129dde9a02c2ad"}

[commit-bot: I haz the power, 2017-01-05 10:44:50]

Description was changed from

==========
[turbofan] Sparse representation for state values

Add a more efficient encoding for state values that have a large number of
optimized-out inputs.
==========

to

==========
[turbofan] Sparse representation for state values

Add a more efficient encoding for state values that have a large number of
optimized-out inputs.

Review-Url: https://codereview.chromium.org/2509623002
Cr-Commit-Position: refs/heads/master@{#42088}
Committed:
https://chromium.googlesource.com/v8/v8/+/68f1a37f8e34e26963f768a8c6129dde9a0...
==========

[commit-bot: I haz the power, 2017-01-05 10:44:51]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/68f1a37f8e34e26963f768a8c6129dde9a0...