[turbofan] Remove minus zero check for rhs of CheckedInt32Sub.

[turbofan] Remove minus zero check for rhs of CheckedInt32Sub.

The only way to get a minus zero result from subtraction is
(-0) - (+0) = -0, hence checking for minus zero on the RHS is
redundant. This is causing some unnecessary deoptimisations
in Box2D from Octane on 32-bit platforms.

BUG=

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

[georgia.kouveli, 2016-10-11 16:33:26]

georgia.kouveli@arm.com changed reviewers:
+ bmeurer@chromium.org, epertoso@chromium.org

[Benedikt Meurer, 2016-10-12 03:36:44]

bmeurer@chromium.org changed reviewers:
+ jarin@chromium.org
- epertoso@chromium.org

[Benedikt Meurer, 2016-10-12 03:36:44]

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

[Benedikt Meurer, 2016-10-12 03:36:44]

Nice catch; this definitely needs a review from Jaro.

I'm not sure this is the best way to do it. I think it would be better to
introduce a truncation that identifies 0 and -0 and pass that for the right hand
side, signaling to the definition that we cannot observe the difference between
0 and -0. But please wait for Jaro to have a look, I think he thought about this
already.

[Jarin, 2016-10-12 05:35:39]

On 2016/10/12 03:36:44, Benedikt Meurer wrote:
> Nice catch; this definitely needs a review from Jaro.
> 
> I'm not sure this is the best way to do it. I think it would be better to
> introduce a truncation that identifies 0 and -0 and pass that for the right
hand
> side, signaling to the definition that we cannot observe the difference
between
> 0 and -0. But please wait for Jaro to have a look, I think he thought about
this
> already.

Indeed, this is a very cool finding.

I second Benedikt's suggestion, we normally propagate observation equivalences
in truncation. I think we should either add a bit to the truncation class
(something like identifies_zeroes or identifies_zero_and_minus_zero if you are
not into the brevity thing), or introduce new truncations to the enum where
relevant (i.e., for Any, Float64, Float32). The advantage of this is that we
could propagate the dont-care-about-minus-zero through other operators, such as
phis, addition etc. E.g., if you have a - (b + c), we could then propagate this
to b and c, and so omit the minus zero check there.

I realize this is more work, do you think you could give it a shot?

FYI, several months ago, we were planning to add the minus zero truncations,
mostly to eliminate some minus zero checks in multiplication. However, we did
not realize it would also help us with our checked operations, so we dropped the
plan. Thanks for doing the actual hard work of analyzing the generated code!

[Jarin, 2016-10-12 07:41:25]

Description was changed from

==========
[turbofan] Remove minus zero check for rhs of CheckedInt32Sub.

The only way to get a minus zero result from subtraction is
(-0) - (+0) = -0, hence checking for minus zero on the RHS is
redundant. This is causing some unnecessary deoptimisations
in Box2D from Octane on 32-bit platforms.

BUG=
==========

to

==========
[turbofan] Remove minus zero check for rhs of CheckedInt32Sub.

The only way to get a minus zero result from subtraction is
(-0) - (+0) = -0, hence checking for minus zero on the RHS is
redundant. This is causing some unnecessary deoptimisations
in Box2D from Octane on 32-bit platforms.

BUG=
==========

[Jarin, 2016-10-12 08:33:07]

Hi Georgia,

we just talked about this here (with @benedikt and @mvstanton), and we realized
we are not completely sure where exactly we want to go with truncations in the
future, so it might be hard for you to navigate those waters.

We do think your solution is a really neat middle-term solution. LGTM.

Thank you!

https://codereview.chromium.org/2410883003/diff/1/src/compiler/representation...
File src/compiler/representation-change.h (right):

https://codereview.chromium.org/2410883003/diff/1/src/compiler/representation...
src/compiler/representation-change.h:212: CheckForMinusZeroMode
minus_zero_check_;
Could you please add the line below?

// TODO(jarin) Integrate with truncations.

[georgia.kouveli, 2016-10-12 09:27:18]

The CQ bit was checked by georgia.kouveli@arm.com

[georgia.kouveli, 2016-10-12 09:27:19]

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/2410883003/#ps20001
(title: "Adding TODO.")

[commit-bot: I haz the power, 2016-10-12 09:27: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, 2016-10-12 09:53:18]

Description was changed from

==========
[turbofan] Remove minus zero check for rhs of CheckedInt32Sub.

The only way to get a minus zero result from subtraction is
(-0) - (+0) = -0, hence checking for minus zero on the RHS is
redundant. This is causing some unnecessary deoptimisations
in Box2D from Octane on 32-bit platforms.

BUG=
==========

to

==========
[turbofan] Remove minus zero check for rhs of CheckedInt32Sub.

The only way to get a minus zero result from subtraction is
(-0) - (+0) = -0, hence checking for minus zero on the RHS is
redundant. This is causing some unnecessary deoptimisations
in Box2D from Octane on 32-bit platforms.

BUG=
==========

[commit-bot: I haz the power, 2016-10-12 09:53:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-12 09:53:38]

Description was changed from

==========
[turbofan] Remove minus zero check for rhs of CheckedInt32Sub.

The only way to get a minus zero result from subtraction is
(-0) - (+0) = -0, hence checking for minus zero on the RHS is
redundant. This is causing some unnecessary deoptimisations
in Box2D from Octane on 32-bit platforms.

BUG=
==========

to

==========
[turbofan] Remove minus zero check for rhs of CheckedInt32Sub.

The only way to get a minus zero result from subtraction is
(-0) - (+0) = -0, hence checking for minus zero on the RHS is
redundant. This is causing some unnecessary deoptimisations
in Box2D from Octane on 32-bit platforms.

BUG=

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

[commit-bot: I haz the power, 2016-10-12 09:53:39]

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