[compiler] merge binary-operator-reducer

[compiler] merge binary-operator-reducer

Merge BinaryOperatorReducer into the MachineOperatorReducer class.
It does not need `Revisit()` calls, because the newly inserted nodes are
visited anyway, and there are no other methods that need AdvancedReducer
there.

BUG=
R=titzer@chromium.org

Committed: https://crrev.com/993ba9d2529a6401b3040b9263f8d06db7dbb4f1
Cr-Commit-Position: refs/heads/master@{#32298}

[fedor.indutny, 2015-11-25 02:00:30]

Hello!

Sorry, but one more patch from me. :)

Thank you,
Fedor.

[titzer, 2015-11-25 09:34:07]

On 2015/11/25 02:00:30, fedor.indutny wrote:
> Hello!
> 
> Sorry, but one more patch from me. :)
> 
> Thank you,
> Fedor.

LGTM, nice that it could be put back in machine-operator-reducer.cc!

[fedor.indutny, 2015-11-25 17:17:25]

Thanks! I have added one more change, but going to hit CQ button anyway. It
seems to be pretty trivial, but if there anything that bothers you - please let
me know and I will file a follow up CL.

[fedor.indutny, 2015-11-25 17:17:29]

The CQ bit was checked by fedor@indutny.com

[fedor.indutny, 2015-11-25 17:17:29]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/1473073004/#ps20001
(title: "range fixes")

[commit-bot: I haz the power, 2015-11-25 17:17:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1473073004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1473073004/20001

[commit-bot: I haz the power, 2015-11-25 19:25:04]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-25 19:25:21]

Description was changed from

==========
[compiler] merge binary-operator-reducer

Merge BinaryOperatorReducer into the MachineOperatorReducer class.
It does not need `Revisit()` calls, because the newly inserted nodes are
visited anyway, and there are no other methods that need AdvancedReducer
there.

BUG=
R=titzer@chromium.org
==========

to

==========
[compiler] merge binary-operator-reducer

Merge BinaryOperatorReducer into the MachineOperatorReducer class.
It does not need `Revisit()` calls, because the newly inserted nodes are
visited anyway, and there are no other methods that need AdvancedReducer
there.

BUG=
R=titzer@chromium.org

Committed: https://crrev.com/993ba9d2529a6401b3040b9263f8d06db7dbb4f1
Cr-Commit-Position: refs/heads/master@{#32298}
==========

[commit-bot: I haz the power, 2015-11-25 19:25:22]

Patchset 2 (id:??) landed as
https://crrev.com/993ba9d2529a6401b3040b9263f8d06db7dbb4f1
Cr-Commit-Position: refs/heads/master@{#32298}

[Benedikt Meurer, 2015-11-25 20:09:10]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2015-11-25 20:09:10]

I'm sorry, but this change is not lgtm. The MachineOperatorReducer must not
access types. Some of that might be implementable as part of the representation
selection. But on the other hand this rule looks rather complex to me. What's
the use case?

https://codereview.chromium.org/1473073004/diff/20001/src/compiler/machine-op...
File src/compiler/machine-operator-reducer.cc (right):

https://codereview.chromium.org/1473073004/diff/20001/src/compiler/machine-op...
src/compiler/machine-operator-reducer.cc:1092: Type* type =
NodeProperties::GetType(node);
The MachineOperatorReducer must not look at the types. The invariant is that it
should function even on a completely untyped graph.

[fedor.indutny, 2015-11-25 21:20:15]

On 2015/11/25 20:09:10, Benedikt Meurer wrote:
> I'm sorry, but this change is not lgtm. The MachineOperatorReducer must not
> access types. Some of that might be implementable as part of the
representation
> selection. But on the other hand this rule looks rather complex to me. What's
> the use case?

Sorry, I didn't know that, but I had a feeling that something might be off
there, considering that there are no other type use in machine-operator-reducer.
There are currently one more use of type information in RoundInt64ToFloat64
reduction, that I have contributed several days ago.

The use case for these rules is performance of big number libraries in
JavaScript. The most efficient way to have them right now is to use 26bit limbs,
because this way addition is quite fast, and multiplication could be done
without a loss of precision. However, there is one problem - multiplication is
using 52bits, which means that both limbs needs to be converted to floats, and
then converted back to 26-bit limbs. This step makes the code 20% slower than it
could be, which is very noticeable when used in ECC cryptography (which is
really multiplication-intensive).

I really need type information to perform this optimization, but perhaps
machine-operator-reducer is not a right place for it. May I ask you to suggest
something that will be conceptually better?

Thank you,
Fedor.

> 
>
https://codereview.chromium.org/1473073004/diff/20001/src/compiler/machine-op...
> File src/compiler/machine-operator-reducer.cc (right):
> 
>
https://codereview.chromium.org/1473073004/diff/20001/src/compiler/machine-op...
> src/compiler/machine-operator-reducer.cc:1092: Type* type =
> NodeProperties::GetType(node);
> The MachineOperatorReducer must not look at the types. The invariant is that
it
> should function even on a completely untyped graph.

[Benedikt Meurer, 2015-11-26 03:49:53]

I'm afraid I'll have to revert those changes. Accessing types in a machine graph
is not safe by design because we don't proper typing for machine operators and
we do non type preserving optimizations. In addition to that we want to go for
concurrent recompilation soonish, and ideally the MachineOperatorReducer would
be able to run concurrently, which means it must not look at the heap, which
means it must not rely on the typed.

I think your optimization can be implemented in an even easier way on the
simplified level. Can you send me samples of JS code where this should kick in?
(via mail)

[Benedikt Meurer, 2015-11-26 03:51:57]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1476763006/ by bmeurer@chromium.org.

The reason for reverting is: Unsound use of types in the MachineOperatorReducer.
Will work on a sound solution with Fedor..

[fedor.indutny, 2015-11-26 03:52:16]

On 2015/11/26 03:49:53, Benedikt Meurer wrote:
> I'm afraid I'll have to revert those changes. Accessing types in a machine
graph
> is not safe by design because we don't proper typing for machine operators and
> we do non type preserving optimizations. In addition to that we want to go for
> concurrent recompilation soonish, and ideally the MachineOperatorReducer would
> be able to run concurrently, which means it must not look at the heap, which
> means it must not rely on the typed.

This is very sad.

> 
> I think your optimization can be implemented in an even easier way on the
> simplified level. Can you send me samples of JS code where this should kick
in?
> (via mail)

Absolutely, it is here:
https://github.com/v8/v8/blob/64efa2a904773816968992628f0bf0f1b7ae82be/test/m...
, other overflowing functions should obviously not be optimized.

I would still love to contribute this to V8 in one form or another, so if you
feel like I could help you with this - please let me know.

Thanks!

[Benedikt Meurer, 2015-11-26 04:03:50]

On 2015/11/26 03:52:16, fedor.indutny wrote:
> On 2015/11/26 03:49:53, Benedikt Meurer wrote:
> > I'm afraid I'll have to revert those changes. Accessing types in a machine
> graph
> > is not safe by design because we don't proper typing for machine operators
and
> > we do non type preserving optimizations. In addition to that we want to go
for
> > concurrent recompilation soonish, and ideally the MachineOperatorReducer
would
> > be able to run concurrently, which means it must not look at the heap, which
> > means it must not rely on the typed.
> 
> This is very sad.
> 
> > 
> > I think your optimization can be implemented in an even easier way on the
> > simplified level. Can you send me samples of JS code where this should kick
> in?
> > (via mail)
> 
> Absolutely, it is here:
>
https://github.com/v8/v8/blob/64efa2a904773816968992628f0bf0f1b7ae82be/test/m...
> , other overflowing functions should obviously not be optimized.
> 
> I would still love to contribute this to V8 in one form or another, so if you
> feel like I could help you with this - please let me know.
> 
> Thanks!

Definitely, we'll make this work. My feeling is that this should be easy(ish)
when done during representation selection. Not sure if we need int64 types for
that tho. I'll talk to jarin@ who's currently refactoring that part.

I'm sorry that you might have wasted some time going in the slightly wrong
direction. I only saw this fly by yesterday. We really value your contribution
to TurboFan (which is not easy these days as a lot of internal knowledge is
required). Thanks

[fedor.indutny, 2015-11-26 04:25:03]

On 2015/11/26 04:03:50, Benedikt Meurer wrote:
> On 2015/11/26 03:52:16, fedor.indutny wrote:
> > On 2015/11/26 03:49:53, Benedikt Meurer wrote:
> > > I'm afraid I'll have to revert those changes. Accessing types in a machine
> > graph
> > > is not safe by design because we don't proper typing for machine operators
> and
> > > we do non type preserving optimizations. In addition to that we want to go
> for
> > > concurrent recompilation soonish, and ideally the MachineOperatorReducer
> would
> > > be able to run concurrently, which means it must not look at the heap,
which
> > > means it must not rely on the typed.
> > 
> > This is very sad.
> > 
> > > 
> > > I think your optimization can be implemented in an even easier way on the
> > > simplified level. Can you send me samples of JS code where this should
kick
> > in?
> > > (via mail)
> > 
> > Absolutely, it is here:
> >
>
https://github.com/v8/v8/blob/64efa2a904773816968992628f0bf0f1b7ae82be/test/m...
> > , other overflowing functions should obviously not be optimized.
> > 
> > I would still love to contribute this to V8 in one form or another, so if
you
> > feel like I could help you with this - please let me know.
> > 
> > Thanks!
> 
> Definitely, we'll make this work. My feeling is that this should be easy(ish)
> when done during representation selection. Not sure if we need int64 types for
> that tho. I'll talk to jarin@ who's currently refactoring that part.
> 
> I'm sorry that you might have wasted some time going in the slightly wrong
> direction. I only saw this fly by yesterday. We really value your contribution
> to TurboFan (which is not easy these days as a lot of internal knowledge is
> required). Thanks

No problem at all, feel free to ping me about it anytime! Thanks!