[turbolizer] Show a label with a shorter parameter for some opcodes.

[turbolizer] Show a label with a shorter parameter for some opcodes.

With this patch, every node in turbo-*.json has an opcode, a title, and
a label. The label field is new; the opcode and title were already
there. The title is for the mouseover text. The label is what will be
displayed in the graph view, unless it's too long, in which case only
the opcode will be displayed. (This is similar to the preexisting
behaviour of putting titles in labels, except that the titles were
rarely short enough to fit in a label.)

With this patch, the labels generated are in practice the same as the
titles we had before, except for LoadField and StoreField, which will be
rendered as LoadField[[+432]] and StoreField[[+432]] (if 432 was the
offset).

This diff adds an overloadable method

    virtual void Operator1<T>::PrintParameter(ostream&, PrintVerbosity)

for each type T to Operator1. Its default implementation just uses
operator<<(ostream&, T const&) and adds square brackets around it, but
it is overridden for FieldAccess to print "[+432]" in the example case.

BUG=
R=jarin,danno

Committed: https://crrev.com/feb93dd60bee1a2853105c2fd154ed07a2b40670
Cr-Commit-Position: refs/heads/master@{#37795}

[bgeron, 2016-06-24 11:35:40]

PTAL.

[danno, 2016-06-27 07:57:00]

https://codereview.chromium.org/2093013002/diff/1/src/compiler/operator.h
File src/compiler/operator.h (right):

https://codereview.chromium.org/2093013002/diff/1/src/compiler/operator.h#new...
src/compiler/operator.h:117: void PrintTo(std::ostream& os, bool verbose = true)
const {
Here and else where, I think it would be better to use an enum  { VERBOSE,
NOT_VERBOSE } for the parameter verbose, rather than a boolean, otherwise it
makes the parameter passing at the calling sites difficult to decipher.

https://codereview.chromium.org/2093013002/diff/1/src/compiler/operator.h#new...
src/compiler/operator.h:124: virtual void PrintTo2(std::ostream& os, bool
verbose) const;
A different name would be better, perhaps PrintToImpl or something that matches
style conventions elsewhere in the code.

https://codereview.chromium.org/2093013002/diff/1/src/compiler/operator.h#new...
src/compiler/operator.h:160: inline void PrintAsParameter(std::ostream& os, T
const& parameter,
I am not quite sure why you need this, since you already have the PrintParameter
method in Operator1 that has a method that should be overridable?

https://codereview.chromium.org/2093013002/diff/1/src/compiler/operator.h#new...
src/compiler/operator.h:162: os << parameter;
Why not return the stream so that PrintAsParameter can be chained just like
other stream operations?

[bgeron, 2016-07-05 13:45:24]

Patchset #2 (id:20001) has been deleted

[bgeron, 2016-07-05 13:54:35]

Thanks; good points. See below.

https://codereview.chromium.org/2093013002/diff/1/src/compiler/operator.h
File src/compiler/operator.h (right):

https://codereview.chromium.org/2093013002/diff/1/src/compiler/operator.h#new...
src/compiler/operator.h:124: virtual void PrintTo2(std::ostream& os, bool
verbose) const;
On 2016/06/27 07:57:00, danno wrote:
> A different name would be better, perhaps PrintToImpl or something that
matches
> style conventions elsewhere in the code.

Done.

https://codereview.chromium.org/2093013002/diff/1/src/compiler/operator.h#new...
src/compiler/operator.h:160: inline void PrintAsParameter(std::ostream& os, T
const& parameter,
On 2016/06/27 07:57:00, danno wrote:
> I am not quite sure why you need this, since you already have the
PrintParameter
> method in Operator1 that has a method that should be overridable?

Good idea. Specializing in simplified-operator.{h,cc} now.

https://codereview.chromium.org/2093013002/diff/1/src/compiler/operator.h#new...
src/compiler/operator.h:162: os << parameter;
On 2016/06/27 07:57:00, danno wrote:
> Why not return the stream so that PrintAsParameter can be chained just like
> other stream operations?

I don't think anyone wants to do that. The syntax "cout << val1 << val2" looks
nice, but "PrintAsParameter(PrintAsParameter(cout, val1), val2)" looks much less
nice.

[danno, 2016-07-06 09:25:47]

getting close!

https://codereview.chromium.org/2093013002/diff/1/src/compiler/operator.h
File src/compiler/operator.h (right):

https://codereview.chromium.org/2093013002/diff/1/src/compiler/operator.h#new...
src/compiler/operator.h:162: os << parameter;
On 2016/07/05 13:54:35, bgeron wrote:
> On 2016/06/27 07:57:00, danno wrote:
> > Why not return the stream so that PrintAsParameter can be chained just like
> > other stream operations?
> 
> I don't think anyone wants to do that. The syntax "cout << val1 << val2" looks
> nice, but "PrintAsParameter(PrintAsParameter(cout, val1), val2)" looks much
less
> nice.

Acknowledged.

https://codereview.chromium.org/2093013002/diff/40001/src/compiler/operator.h
File src/compiler/operator.h (left):

https://codereview.chromium.org/2093013002/diff/40001/src/compiler/operator.h...
src/compiler/operator.h:148: 
Please remove stray change.

https://codereview.chromium.org/2093013002/diff/40001/src/compiler/operator.h
File src/compiler/operator.h (right):

https://codereview.chromium.org/2093013002/diff/40001/src/compiler/operator.h...
src/compiler/operator.h:53: enum class Verbosity { kVerbose, kSilent };
Please call this PrintVerbosity. You should be able to elide the actual name
almost everywhere, since Operator::kVerbose/kSilent should be adequate
qualification in most places.

https://codereview.chromium.org/2093013002/diff/40001/src/compiler/simplified...
File src/compiler/simplified-operator.cc (right):

https://codereview.chromium.org/2093013002/diff/40001/src/compiler/simplified...
src/compiler/simplified-operator.cc:130: if (verbose == Verbosity::kVerbose) {
Here and elsewhere, you can elide the "Verbosity". Operator should be enough.

[bgeron, 2016-07-13 10:55:14]

Description was changed from

==========
[turbolizer] Show a label with a shorter parameter for some opcodes.

With this patch, every node in turbo-*.json has an opcode, a title, and
a label. The label field is new; the opcode and title were already
there. The title is for the mouseover text. The label is what will be
displayed in the graph view, unless it's too long, in which case only
the opcode will be displayed. (This is similar to the preexisting
behaviour of putting titles in labels, except that the titles were
rarely short enough to fit in a label.)

With this patch, the labels generated are in practice the same as the
titles we had before, except for LoadField and StoreField, which will be
rendered as LoadField[[+432]] and StoreField[[+432]] (if 432 was the
offset).

This diff adds an overloadable function

    PrintAsParameter(ostream& os, T const& val, bool verbose)

for every type T. Its default implementation is just "os << val", but it
is overridden for FieldAccess to print "[+432]" in the example case.

BUG=
R=jarin,danno
==========

to

==========
[turbolizer] Show a label with a shorter parameter for some opcodes.

With this patch, every node in turbo-*.json has an opcode, a title, and
a label. The label field is new; the opcode and title were already
there. The title is for the mouseover text. The label is what will be
displayed in the graph view, unless it's too long, in which case only
the opcode will be displayed. (This is similar to the preexisting
behaviour of putting titles in labels, except that the titles were
rarely short enough to fit in a label.)

With this patch, the labels generated are in practice the same as the
titles we had before, except for LoadField and StoreField, which will be
rendered as LoadField[[+432]] and StoreField[[+432]] (if 432 was the
offset).

This diff adds an overloadable method

    virtual void Operator1<T>::PrintParameter(ostream&, PrintVerbosity)

for each type T to Operator1. Its default implementation just uses
operator<<(ostream&, T const&) and adds square brakcets around it, but it is
overridden for FieldAccess to print "[+432]" in the example case.

BUG=
R=jarin,danno
==========

[bgeron, 2016-07-13 11:08:06]

See below. Also, I fixed the printing behaviour of subclasses of Operator1<T>,
such as CallOperator.

https://codereview.chromium.org/2093013002/diff/40001/src/compiler/operator.h
File src/compiler/operator.h (left):

https://codereview.chromium.org/2093013002/diff/40001/src/compiler/operator.h...
src/compiler/operator.h:148: 
On 2016/07/06 09:25:47, danno wrote:
> Please remove stray change.

Done.

https://codereview.chromium.org/2093013002/diff/40001/src/compiler/operator.h
File src/compiler/operator.h (right):

https://codereview.chromium.org/2093013002/diff/40001/src/compiler/operator.h...
src/compiler/operator.h:53: enum class Verbosity { kVerbose, kSilent };
On 2016/07/06 09:25:47, danno wrote:
> Please call this PrintVerbosity. You should be able to elide the actual name
> almost everywhere, since Operator::kVerbose/kSilent should be adequate
> qualification in most places.

Done.

As I made it an enum class not an enum, we have to write
PrintVerbosity::kVerbose, but I don't think it's too verbose as is.

https://codereview.chromium.org/2093013002/diff/40001/src/compiler/simplified...
File src/compiler/simplified-operator.cc (right):

https://codereview.chromium.org/2093013002/diff/40001/src/compiler/simplified...
src/compiler/simplified-operator.cc:130: if (verbose == Verbosity::kVerbose) {
On 2016/07/06 09:25:47, danno wrote:
> Here and elsewhere, you can elide the "Verbosity". Operator should be enough.

Replaced Operator::Verbosity by PrintVerbosity. Is that what you meant?

[danno, 2016-07-13 12:56:57]

lgtm

[bgeron, 2016-07-13 14:21:01]

The CQ bit was checked by bgeron@google.com

[commit-bot: I haz the power, 2016-07-13 14:21:05]

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

[bgeron, 2016-07-13 14:21:06]

The CQ bit was unchecked by bgeron@google.com

[bgeron, 2016-07-13 14:21:29]

The CQ bit was checked by bgeron@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 14:21:35]

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-07-13 14:22:56]

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

[commit-bot: I haz the power, 2016-07-13 14:22:57]

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_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/19360)

[bgeron, 2016-07-13 14:36:14]

The CQ bit was checked by bgeron@google.com

[bgeron, 2016-07-13 14:36:15]

The patchset sent to the CQ was uploaded after l-g-t-m from danno@chromium.org
Link to the patchset: https://codereview.chromium.org/2093013002/#ps80001
(title: "Merge with master")

[commit-bot: I haz the power, 2016-07-13 14:36:25]

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

[bgeron, 2016-07-13 14:43:15]

Description was changed from

==========
[turbolizer] Show a label with a shorter parameter for some opcodes.

With this patch, every node in turbo-*.json has an opcode, a title, and
a label. The label field is new; the opcode and title were already
there. The title is for the mouseover text. The label is what will be
displayed in the graph view, unless it's too long, in which case only
the opcode will be displayed. (This is similar to the preexisting
behaviour of putting titles in labels, except that the titles were
rarely short enough to fit in a label.)

With this patch, the labels generated are in practice the same as the
titles we had before, except for LoadField and StoreField, which will be
rendered as LoadField[[+432]] and StoreField[[+432]] (if 432 was the
offset).

This diff adds an overloadable method

    virtual void Operator1<T>::PrintParameter(ostream&, PrintVerbosity)

for each type T to Operator1. Its default implementation just uses
operator<<(ostream&, T const&) and adds square brakcets around it, but it is
overridden for FieldAccess to print "[+432]" in the example case.

BUG=
R=jarin,danno
==========

to

==========
[turbolizer] Show a label with a shorter parameter for some opcodes.

With this patch, every node in turbo-*.json has an opcode, a title, and
a label. The label field is new; the opcode and title were already
there. The title is for the mouseover text. The label is what will be
displayed in the graph view, unless it's too long, in which case only
the opcode will be displayed. (This is similar to the preexisting
behaviour of putting titles in labels, except that the titles were
rarely short enough to fit in a label.)

With this patch, the labels generated are in practice the same as the
titles we had before, except for LoadField and StoreField, which will be
rendered as LoadField[[+432]] and StoreField[[+432]] (if 432 was the
offset).

This diff adds an overloadable method

    virtual void Operator1<T>::PrintParameter(ostream&, PrintVerbosity)

for each type T to Operator1. Its default implementation just uses
operator<<(ostream&, T const&) and adds square brakcets around it, but
it is overridden for FieldAccess to print "[+432]" in the example case.

BUG=
R=jarin,danno
==========

[bgeron, 2016-07-13 14:43:30]

Description was changed from

==========
[turbolizer] Show a label with a shorter parameter for some opcodes.

With this patch, every node in turbo-*.json has an opcode, a title, and
a label. The label field is new; the opcode and title were already
there. The title is for the mouseover text. The label is what will be
displayed in the graph view, unless it's too long, in which case only
the opcode will be displayed. (This is similar to the preexisting
behaviour of putting titles in labels, except that the titles were
rarely short enough to fit in a label.)

With this patch, the labels generated are in practice the same as the
titles we had before, except for LoadField and StoreField, which will be
rendered as LoadField[[+432]] and StoreField[[+432]] (if 432 was the
offset).

This diff adds an overloadable method

    virtual void Operator1<T>::PrintParameter(ostream&, PrintVerbosity)

for each type T to Operator1. Its default implementation just uses
operator<<(ostream&, T const&) and adds square brakcets around it, but
it is overridden for FieldAccess to print "[+432]" in the example case.

BUG=
R=jarin,danno
==========

to

==========
[turbolizer] Show a label with a shorter parameter for some opcodes.

With this patch, every node in turbo-*.json has an opcode, a title, and
a label. The label field is new; the opcode and title were already
there. The title is for the mouseover text. The label is what will be
displayed in the graph view, unless it's too long, in which case only
the opcode will be displayed. (This is similar to the preexisting
behaviour of putting titles in labels, except that the titles were
rarely short enough to fit in a label.)

With this patch, the labels generated are in practice the same as the
titles we had before, except for LoadField and StoreField, which will be
rendered as LoadField[[+432]] and StoreField[[+432]] (if 432 was the
offset).

This diff adds an overloadable method

    virtual void Operator1<T>::PrintParameter(ostream&, PrintVerbosity)

for each type T to Operator1. Its default implementation just uses
operator<<(ostream&, T const&) and adds square brackets around it, but
it is overridden for FieldAccess to print "[+432]" in the example case.

BUG=
R=jarin,danno
==========

[commit-bot: I haz the power, 2016-07-13 14:43:35]

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

[commit-bot: I haz the power, 2016-07-13 14:43:36]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/19364)

[bgeron, 2016-07-13 14:46:11]

bgeron@google.com changed reviewers:
- danno@chromium.org

[bgeron, 2016-07-13 14:46:12]

Jaro, I need another LGTM from an owner. What do you think?

Cheers, Bram

[Jarin, 2016-07-15 11:05:17]

lgtm

[bgeron, 2016-07-15 11:41:26]

The CQ bit was checked by bgeron@google.com

[commit-bot: I haz the power, 2016-07-15 11:41:36]

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-07-15 12:05:57]

Description was changed from

==========
[turbolizer] Show a label with a shorter parameter for some opcodes.

With this patch, every node in turbo-*.json has an opcode, a title, and
a label. The label field is new; the opcode and title were already
there. The title is for the mouseover text. The label is what will be
displayed in the graph view, unless it's too long, in which case only
the opcode will be displayed. (This is similar to the preexisting
behaviour of putting titles in labels, except that the titles were
rarely short enough to fit in a label.)

With this patch, the labels generated are in practice the same as the
titles we had before, except for LoadField and StoreField, which will be
rendered as LoadField[[+432]] and StoreField[[+432]] (if 432 was the
offset).

This diff adds an overloadable method

    virtual void Operator1<T>::PrintParameter(ostream&, PrintVerbosity)

for each type T to Operator1. Its default implementation just uses
operator<<(ostream&, T const&) and adds square brackets around it, but
it is overridden for FieldAccess to print "[+432]" in the example case.

BUG=
R=jarin,danno
==========

to

==========
[turbolizer] Show a label with a shorter parameter for some opcodes.

With this patch, every node in turbo-*.json has an opcode, a title, and
a label. The label field is new; the opcode and title were already
there. The title is for the mouseover text. The label is what will be
displayed in the graph view, unless it's too long, in which case only
the opcode will be displayed. (This is similar to the preexisting
behaviour of putting titles in labels, except that the titles were
rarely short enough to fit in a label.)

With this patch, the labels generated are in practice the same as the
titles we had before, except for LoadField and StoreField, which will be
rendered as LoadField[[+432]] and StoreField[[+432]] (if 432 was the
offset).

This diff adds an overloadable method

    virtual void Operator1<T>::PrintParameter(ostream&, PrintVerbosity)

for each type T to Operator1. Its default implementation just uses
operator<<(ostream&, T const&) and adds square brackets around it, but
it is overridden for FieldAccess to print "[+432]" in the example case.

BUG=
R=jarin,danno
==========

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

Committed patchset #4 (id:80001)

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

Description was changed from

==========
[turbolizer] Show a label with a shorter parameter for some opcodes.

With this patch, every node in turbo-*.json has an opcode, a title, and
a label. The label field is new; the opcode and title were already
there. The title is for the mouseover text. The label is what will be
displayed in the graph view, unless it's too long, in which case only
the opcode will be displayed. (This is similar to the preexisting
behaviour of putting titles in labels, except that the titles were
rarely short enough to fit in a label.)

With this patch, the labels generated are in practice the same as the
titles we had before, except for LoadField and StoreField, which will be
rendered as LoadField[[+432]] and StoreField[[+432]] (if 432 was the
offset).

This diff adds an overloadable method

    virtual void Operator1<T>::PrintParameter(ostream&, PrintVerbosity)

for each type T to Operator1. Its default implementation just uses
operator<<(ostream&, T const&) and adds square brackets around it, but
it is overridden for FieldAccess to print "[+432]" in the example case.

BUG=
R=jarin,danno
==========

to

==========
[turbolizer] Show a label with a shorter parameter for some opcodes.

With this patch, every node in turbo-*.json has an opcode, a title, and
a label. The label field is new; the opcode and title were already
there. The title is for the mouseover text. The label is what will be
displayed in the graph view, unless it's too long, in which case only
the opcode will be displayed. (This is similar to the preexisting
behaviour of putting titles in labels, except that the titles were
rarely short enough to fit in a label.)

With this patch, the labels generated are in practice the same as the
titles we had before, except for LoadField and StoreField, which will be
rendered as LoadField[[+432]] and StoreField[[+432]] (if 432 was the
offset).

This diff adds an overloadable method

    virtual void Operator1<T>::PrintParameter(ostream&, PrintVerbosity)

for each type T to Operator1. Its default implementation just uses
operator<<(ostream&, T const&) and adds square brackets around it, but
it is overridden for FieldAccess to print "[+432]" in the example case.

BUG=
R=jarin,danno

Committed: https://crrev.com/feb93dd60bee1a2853105c2fd154ed07a2b40670
Cr-Commit-Position: refs/heads/master@{#37795}
==========

[commit-bot: I haz the power, 2016-07-15 12:07:54]

Patchset 4 (id:??) landed as
https://crrev.com/feb93dd60bee1a2853105c2fd154ed07a2b40670
Cr-Commit-Position: refs/heads/master@{#37795}