[compiler] Introduce a simple store-store elimination, disabled by default.

[compiler] Introduce a simple store-store elimination, disabled by default.

R=jarin@chromium.org
BUG=

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

[bgeron, 2016-06-21 12:45:13]

Please take a look.

[Jarin, 2016-06-21 13:28:33]

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

https://codereview.chromium.org/2087483003/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:1050: NodeVector eligible(temp_zone);
Could we move this code into the StoreStoreElimination class? E.g., you could
have a Run method there that does all this.

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
File src/compiler/store-store-elimination.cc (right):

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:15: // A pretty dumb store-store
elimination. When the effect chain contains the
"pretty dumb" --> simple

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:74: typedef uint16_t Offset;
Please, put the file-local definitions into an anonymous namespace. Then you do
not even have to use static. Something like:

namespace {

typedef ...;

Offset toOffset(int offset) { ... }

bool IsEndOfStoreFieldChain(Node* node) { ... }

}  // namespace

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:78: Offset toOffset(int offset) {
toOffset -> ToOffset

(link to style guide: https://www.chromium.org/developers/coding-style)

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:151: // Try to insert. If it was
present, this will preserve the original value.
You should handle field aliasing in some way. Perhaps the easiest way is to only
do store elimination for tagged values
(field_access.machine_type.representation() == MachineRepresentation::kTagged) -
those are aligned and we should never try to store to only some bytes of the
value or some such.

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:170: // the chain.
Note: Actually, I believe you could have a multimap and store both objects for
the offset. In fact, I think you could have a set of <field, object> pairs and
eliminate the store if you find the same pair. No?

However, we should only do this later.

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:184: Node* previous =
PreviousEffectBeforeStoreField(current_node);
Can't you move this line to the beginning of the loop (e.g., line 150), replace
the line 211 with 200 and remove lines 197 - 205.

Then you could even turn the loop condition to true (or make it

do { ... 
} while (current_node != nullptr && 
         current_node->op()->opcode() == IrOpcode::kStoreField);

What do you think?

https://codereview.chromium.org/2087483003/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2087483003/diff/1/src/flag-definitions.h#newc...
src/flag-definitions.h:449: DEFINE_BOOL(trace_turbo_ssel, false, "trace
TurboFan's store-store elimination")
Can't you just reuse the trace_store_elimination flag?

Also, you should introduce turbo_store_elimination flag that turns off the store
elimination. This is useful if we need to turn it of because of a bug or
performance regression.

[bgeron, 2016-06-21 17:48:59]

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

https://codereview.chromium.org/2087483003/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:1050: NodeVector eligible(temp_zone);
On 2016/06/21 13:28:32, Jarin wrote:
> Could we move this code into the StoreStoreElimination class? E.g., you could
> have a Run method there that does all this.

Done.

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
File src/compiler/store-store-elimination.cc (right):

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:15: // A pretty dumb store-store
elimination. When the effect chain contains the
On 2016/06/21 13:28:33, Jarin wrote:
> "pretty dumb" --> simple

Much better.. Would be good to keep the codebase fully PC :D

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:74: typedef uint16_t Offset;
On 2016/06/21 13:28:32, Jarin wrote:
> Please, put the file-local definitions into an anonymous namespace. Then you
do
> not even have to use static. Something like:
> 
> namespace {
> 
> typedef ...;
> 
> Offset toOffset(int offset) { ... }
> 
> bool IsEndOfStoreFieldChain(Node* node) { ... }
> 
> }  // namespace

Done.

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:78: Offset toOffset(int offset) {
On 2016/06/21 13:28:33, Jarin wrote:
> toOffset -> ToOffset
> 
> (link to style guide: https://www.chromium.org/developers/coding-style)

Done.

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:151: // Try to insert. If it was
present, this will preserve the original value.
On 2016/06/21 13:28:33, Jarin wrote:
> You should handle field aliasing in some way. Perhaps the easiest way is to
only
> do store elimination for tagged values
> (field_access.machine_type.representation() == MachineRepresentation::kTagged)
-
> those are aligned and we should never try to store to only some bytes of the
> value or some such.

Is there more information on field aliasing? I could add a check, but I don't
know how to generate input that exercises it so I can't hand-test this.

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:170: // the chain.
On 2016/06/21 13:28:33, Jarin wrote:
> Note: Actually, I believe you could have a multimap and store both objects for
> the offset. In fact, I think you could have a set of <field, object> pairs and
> eliminate the store if you find the same pair. No?
> 
> However, we should only do this later.

I think you're right!

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:184: Node* previous =
PreviousEffectBeforeStoreField(current_node);
On 2016/06/21 13:28:32, Jarin wrote:
> Can't you move this line to the beginning of the loop (e.g., line 150),
replace
> the line 211 with 200 and remove lines 197 - 205.
> 
> Then you could even turn the loop condition to true (or make it
> 
> do { ... 
> } while (current_node != nullptr && 
>          current_node->op()->opcode() == IrOpcode::kStoreField);
> 
> What do you think?

Great idea, I missed that.

https://codereview.chromium.org/2087483003/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2087483003/diff/1/src/flag-definitions.h#newc...
src/flag-definitions.h:449: DEFINE_BOOL(trace_turbo_ssel, false, "trace
TurboFan's store-store elimination")
On 2016/06/21 13:28:33, Jarin wrote:
> Can't you just reuse the trace_store_elimination flag?

Sounds a good idea. I feel we ought to move it from the Crankshaft section to a
more general section, but where would that be?

> Also, you should introduce turbo_store_elimination flag that turns off the
store
> elimination. This is useful if we need to turn it of because of a bug or
> performance regression.

Done. Note that we now have both FLAG_store_elimination and
FLAG_turbo_store_elimination, but I guess it's better that way because the first
one is off by default.

[bgeron, 2016-06-22 09:15:11]

Please use the difference from patch set 1 to patch set 4 for re-reviewing.

I also renamed StoreStoreElimination::zone_ to StoreStoreEliminaton::temp_zone_
to better reflect its purpose.

[bgeron, 2016-06-22 18:00:58]

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
File src/compiler/store-store-elimination.cc (right):

https://codereview.chromium.org/2087483003/diff/1/src/compiler/store-store-el...
src/compiler/store-store-elimination.cc:151: // Try to insert. If it was
present, this will preserve the original value.
On 2016/06/21 17:48:58, bgeron wrote:
> On 2016/06/21 13:28:33, Jarin wrote:
> > You should handle field aliasing in some way. Perhaps the easiest way is to
> only
> > do store elimination for tagged values
> > (field_access.machine_type.representation() ==
MachineRepresentation::kTagged)
> -
> > those are aligned and we should never try to store to only some bytes of the
> > value or some such.
> 
> Is there more information on field aliasing? I could add a check, but I don't
> know how to generate input that exercises it so I can't hand-test this.

Done.

https://codereview.chromium.org/2087483003/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2087483003/diff/1/src/flag-definitions.h#newc...
src/flag-definitions.h:449: DEFINE_BOOL(trace_turbo_ssel, false, "trace
TurboFan's store-store elimination")
On 2016/06/21 17:48:59, bgeron wrote:
> On 2016/06/21 13:28:33, Jarin wrote:
> > Can't you just reuse the trace_store_elimination flag?
> 
> Sounds a good idea. I feel we ought to move it from the Crankshaft section to
a
> more general section, but where would that be?
> 
> > Also, you should introduce turbo_store_elimination flag that turns off the
> store
> > elimination. This is useful if we need to turn it of because of a bug or
> > performance regression.
> 
> Done. Note that we now have both FLAG_store_elimination and
> FLAG_turbo_store_elimination, but I guess it's better that way because the
first
> one is off by default.

Done.

[bgeron, 2016-06-22 18:01:10]

Against your recommendation I added a method MachineType::IsWidestSize(void) in
machine-type.h, because although SIMD types are not activated, they are
partially in the codebase. This way it is clearer that they are currently
deactivated. Please advise.

[bgeron, 2016-06-22 18:04:00]

Need to fix upstream to satisfy the trybots; will do tomorrow.

[Jarin, 2016-06-23 09:58:29]

lgtm

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

https://codereview.chromium.org/2087483003/diff/100001/src/compiler/pipeline....
src/compiler/pipeline.cc:12: #include "src/compiler/all-nodes.h"
You should not need to add that.

https://codereview.chromium.org/2087483003/diff/100001/src/compiler/store-sto...
File src/compiler/store-store-elimination.cc (right):

https://codereview.chromium.org/2087483003/diff/100001/src/compiler/store-sto...
src/compiler/store-store-elimination.cc:178: printf("**
StoreStoreElimination::ReduceEligibleNode activated: #%d\n",
printf -> PrintF (here and elsewhere)

https://codereview.chromium.org/2087483003/diff/100001/src/compiler/store-sto...
src/compiler/store-store-elimination.cc:181: auto log = [](NodeId id, Offset
off, const char* msg) {
This does not have to be a lambda, please put it into a separate function.

https://codereview.chromium.org/2087483003/diff/100001/src/compiler/store-sto...
src/compiler/store-store-elimination.cc:202: if
(access.machine_type.IsWidestSize()) {
I still think this is not a good idea. If someone introduces a type that is
wider than the tagged pointer, your code will stop working without anyone
noticing.

It would be better to CHECK that the size is at most tagged and only eliminate
if the size is the same as kPointerSize.

[bgeron, 2016-06-23 12:03:05]

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

https://codereview.chromium.org/2087483003/diff/100001/src/compiler/pipeline....
src/compiler/pipeline.cc:12: #include "src/compiler/all-nodes.h"
On 2016/06/23 09:58:28, Jarin wrote:
> You should not need to add that.

Done.

https://codereview.chromium.org/2087483003/diff/100001/src/compiler/store-sto...
File src/compiler/store-store-elimination.cc (right):

https://codereview.chromium.org/2087483003/diff/100001/src/compiler/store-sto...
src/compiler/store-store-elimination.cc:178: printf("**
StoreStoreElimination::ReduceEligibleNode activated: #%d\n",
On 2016/06/23 09:58:29, Jarin wrote:
> printf -> PrintF (here and elsewhere)

Done.

https://codereview.chromium.org/2087483003/diff/100001/src/compiler/store-sto...
src/compiler/store-store-elimination.cc:181: auto log = [](NodeId id, Offset
off, const char* msg) {
On 2016/06/23 09:58:28, Jarin wrote:
> This does not have to be a lambda, please put it into a separate function.

Done.

https://codereview.chromium.org/2087483003/diff/100001/src/compiler/store-sto...
src/compiler/store-store-elimination.cc:202: if
(access.machine_type.IsWidestSize()) {
On 2016/06/23 09:58:28, Jarin wrote:
> I still think this is not a good idea. If someone introduces a type that is
> wider than the tagged pointer, your code will stop working without anyone
> noticing.
> 
> It would be better to CHECK that the size is at most tagged and only eliminate
> if the size is the same as kPointerSize.

Done.

[bgeron, 2016-06-23 12:03:35]

Description was changed from

==========
[compiler] Introduce a simple store-store elimination.

R=jarin@chromium.org
BUG=
==========

to

==========
[compiler] Introduce a simple store-store elimination, disabled by default.

R=jarin@chromium.org
BUG=
==========

[bgeron, 2016-06-23 14:31:00]

On 2016/06/23 12:03:05, bgeron wrote:
>
https://codereview.chromium.org/2087483003/diff/100001/src/compiler/pipeline.cc
> File src/compiler/pipeline.cc (right):
> 
>
https://codereview.chromium.org/2087483003/diff/100001/src/compiler/pipeline....
> src/compiler/pipeline.cc:12: #include "src/compiler/all-nodes.h"
> On 2016/06/23 09:58:28, Jarin wrote:
> > You should not need to add that.
> 
> Done.
> 
>
https://codereview.chromium.org/2087483003/diff/100001/src/compiler/store-sto...
> File src/compiler/store-store-elimination.cc (right):
> 
>
https://codereview.chromium.org/2087483003/diff/100001/src/compiler/store-sto...
> src/compiler/store-store-elimination.cc:178: printf("**
> StoreStoreElimination::ReduceEligibleNode activated: #%d\n",
> On 2016/06/23 09:58:29, Jarin wrote:
> > printf -> PrintF (here and elsewhere)
> 
> Done.
> 
>
https://codereview.chromium.org/2087483003/diff/100001/src/compiler/store-sto...
> src/compiler/store-store-elimination.cc:181: auto log = [](NodeId id, Offset
> off, const char* msg) {
> On 2016/06/23 09:58:28, Jarin wrote:
> > This does not have to be a lambda, please put it into a separate function.
> 
> Done.
> 
>
https://codereview.chromium.org/2087483003/diff/100001/src/compiler/store-sto...
> src/compiler/store-store-elimination.cc:202: if
> (access.machine_type.IsWidestSize()) {
> On 2016/06/23 09:58:28, Jarin wrote:
> > I still think this is not a good idea. If someone introduces a type that is
> > wider than the tagged pointer, your code will stop working without anyone
> > noticing.
> > 
> > It would be better to CHECK that the size is at most tagged and only
eliminate
> > if the size is the same as kPointerSize.
> 
> Done.

All tests succeed with the following command line:

    tools/run-tests.py --buildbot --mode=Optdebug --arch=x64
--extra-flags='--turbo-store-elimination --no-trace-turbo'

[bgeron, 2016-06-23 14:31:06]

The CQ bit was checked by bgeron@google.com

[bgeron, 2016-06-23 14:31:07]

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/2087483003/#ps120001
(title: "Address remaining concerns.")

[commit-bot: I haz the power, 2016-06-23 14:31:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2087483003/120001

[bgeron, 2016-06-23 14:34:29]

The CQ bit was unchecked by bgeron@google.com

[bgeron, 2016-06-23 14:47:48]

The CQ bit was checked by bgeron@google.com

[bgeron, 2016-06-23 14:47:49]

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/2087483003/#ps140001
(title: "Explicitly promote constant argument of <<, to satisfy MSVC.")

[commit-bot: I haz the power, 2016-06-23 14:47:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2087483003/140001

[commit-bot: I haz the power, 2016-06-23 15:13:41]

Description was changed from

==========
[compiler] Introduce a simple store-store elimination, disabled by default.

R=jarin@chromium.org
BUG=
==========

to

==========
[compiler] Introduce a simple store-store elimination, disabled by default.

R=jarin@chromium.org
BUG=
==========

[commit-bot: I haz the power, 2016-06-23 15:13:42]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-23 15:17:53]

Description was changed from

==========
[compiler] Introduce a simple store-store elimination, disabled by default.

R=jarin@chromium.org
BUG=
==========

to

==========
[compiler] Introduce a simple store-store elimination, disabled by default.

R=jarin@chromium.org
BUG=

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

[commit-bot: I haz the power, 2016-06-23 15:17:54]

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