[Turbofan]: Eliminate the check for -0 if it's not possible/observable.

[Turbofan]: Eliminate the check for -0 if it's not possible/observable.

In int32 multiplication, if we have a positive integer as input, then we know we can't produce a -0 answer. The same is true if truncation is applied (x * y | 0). Without this information, we have to rather annoyingly check if the result of multiplication is 0, then OR the inputs to check for negativity, and possibly return -0. In TurboFan, we'll deopt in this case.

BUG=

Committed: https://crrev.com/47aaac644216177be86f93f57c9fb3e1ed23dda3
Cr-Commit-Position: refs/heads/master@{#37831}

[mvstanton, 2016-07-17 16:21:19]

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

[mvstanton, 2016-07-17 16:21:19]

Hi Benedikt,
Here is the elimination of the -0 case in integer multiplication. Have a look,
thanks!
--Mike

[mvstanton, 2016-07-17 16:21:28]

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

[commit-bot: I haz the power, 2016-07-17 16:21:40]

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

[Jarin, 2016-07-17 16:37:47]

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

[Jarin, 2016-07-17 16:37:47]

lgtm with nits.

Was not there a test that checked that we stay optimized even if we multiply
with -0 in the truncated case? I vaguely remember that it failed after the int32
mul change, but I don't remember what happened to it. Have we disabled it? If
yes, maybe it is time to enable.

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/representa...
File src/compiler/representation-change.cc (right):

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/representa...
src/compiler/representation-change.cc:634:
CheckForMinusZeroMode::kCheckForMinusZero);
Just remove this case.

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/simplified...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1346: (input1_type->IsRange() &&
input1_type->AsRange()->Min() > 0)) {
Maybe it is better to not refer to ranges here and just say:

type->Is(Type::OrderedNumber()) && type->Min() > 0

[commit-bot: I haz the power, 2016-07-17 16:44:01]

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

[commit-bot: I haz the power, 2016-07-17 16:44:02]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-07-17 17:44:08]

Nice. LGTM once comments addressed.

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/effect-con...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/effect-con...
src/compiler/effect-control-linearizer.cc:1325: if (mode ==
CheckForMinusZeroMode::kDontCheckForMinusZero) {
Nit: I'd prefer a positive check here, i.e.

if (mode == CheckForMinusZeroMode::kCheckForMinusZero) {
  ...
}
return ValueEffectControl(value, effect, control);

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/simplified...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1346: (input1_type->IsRange() &&
input1_type->AsRange()->Min() > 0)) {
Yes, please. And I'd prefer to have this inline below instead of the bool
variable.

https://codereview.chromium.org/2154073002/diff/20001/test/mjsunit/math-mul.js
File test/mjsunit/math-mul.js (right):

https://codereview.chromium.org/2154073002/diff/20001/test/mjsunit/math-mul.j...
test/mjsunit/math-mul.js:1: // Copyright 2016 the V8 project authors. All rights
reserved.
Can you move this file into mjsunit/compiler/ directory?

[Benedikt Meurer, 2016-07-17 17:44:57]

Also can you update the description to add more details about the change.
That'll make it easier to trace back issues later.

[Jarin, 2016-07-17 18:24:41]

Now that I have looked a bit more carefully, I have couple more suggestions.

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/simplified...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1343: Type* input1_type =
NodeProperties::GetType(node->InputAt(1));
You can use TypeOf here (instead of NodeProperties::GetType).

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1355: :
CheckForMinusZeroMode::kCheckForMinusZero;
Looking at this again, this logic is only used when lowering and only if we are
changing to Int32MulWithOverflow.

Perhaps it would be better to move this into ChangeToInt32MulWithOverflowOp?
(And perhaps rename to LowerToInt32MulWithOverflow.)

WDYT?

[mvstanton, 2016-07-18 08:48:23]

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

[commit-bot: I haz the power, 2016-07-18 08:48:30]

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

[mvstanton, 2016-07-18 08:48:58]

Thanks guys, comments addressed.

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/effect-con...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/effect-con...
src/compiler/effect-control-linearizer.cc:1325: if (mode ==
CheckForMinusZeroMode::kDontCheckForMinusZero) {
On 2016/07/17 17:44:07, Benedikt Meurer wrote:
> Nit: I'd prefer a positive check here, i.e.
> 
> if (mode == CheckForMinusZeroMode::kCheckForMinusZero) {
>   ...
> }
> return ValueEffectControl(value, effect, control);

Done.

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/representa...
File src/compiler/representation-change.cc (right):

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/representa...
src/compiler/representation-change.cc:634:
CheckForMinusZeroMode::kCheckForMinusZero);
On 2016/07/17 16:37:47, Jarin wrote:
> Just remove this case.

Done.

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/simplified...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1343: Type* input1_type =
NodeProperties::GetType(node->InputAt(1));
On 2016/07/17 18:24:41, Jarin wrote:
> You can use TypeOf here (instead of NodeProperties::GetType).

Done.

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1346: (input1_type->IsRange() &&
input1_type->AsRange()->Min() > 0)) {
On 2016/07/17 16:37:47, Jarin wrote:
> Maybe it is better to not refer to ranges here and just say:
> 
> type->Is(Type::OrderedNumber()) && type->Min() > 0

Done.

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1346: (input1_type->IsRange() &&
input1_type->AsRange()->Min() > 0)) {
On 2016/07/17 17:44:07, Benedikt Meurer wrote:
> Yes, please. And I'd prefer to have this inline below instead of the bool
> variable.

No problemo

https://codereview.chromium.org/2154073002/diff/20001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1355: :
CheckForMinusZeroMode::kCheckForMinusZero;
On 2016/07/17 18:24:41, Jarin wrote:
> Looking at this again, this logic is only used when lowering and only if we
are
> changing to Int32MulWithOverflow.
> 
> Perhaps it would be better to move this into ChangeToInt32MulWithOverflowOp?
> (And perhaps rename to LowerToInt32MulWithOverflow.)
> 
> WDYT?

Good idea, done!

https://codereview.chromium.org/2154073002/diff/20001/test/mjsunit/math-mul.js
File test/mjsunit/math-mul.js (right):

https://codereview.chromium.org/2154073002/diff/20001/test/mjsunit/math-mul.j...
test/mjsunit/math-mul.js:1: // Copyright 2016 the V8 project authors. All rights
reserved.
On 2016/07/17 17:44:07, Benedikt Meurer wrote:
> Can you move this file into mjsunit/compiler/ directory?

Done.

[mvstanton, 2016-07-18 08:52:09]

Description was changed from

==========
[Turbofan]: Eliminate the check for -0 if it's not possible/observable.

(In int32 multiplication)

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

to

==========
[Turbofan]: Eliminate the check for -0 if it's not possible/observable.

In int32 multiplication, if we have a positive integer as input, then we know we
can't produce a -0 answer. The same is true if truncation is applied (x * y |
0). Without this information, we have to rather annoyingly check if the result
of multiplication is 0, then OR the inputs to check for negativity, and possibly
return -0. In TurboFan, we'll deopt in this case.


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

[mvstanton, 2016-07-18 08:52:23]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2016-07-18 08:52:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2154073002/#ps40001
(title: "Code comments.")

[commit-bot: I haz the power, 2016-07-18 08:52:29]

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

[Benedikt Meurer, 2016-07-18 08:55:38]

Still LGTM (with new nit).

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

https://codereview.chromium.org/2154073002/diff/40001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1080: void
LowerToCheckedInt32MulWithOverflowOp(Node* node, Truncation truncation,
Nit: Just LowerToCheckedInt32Mul.

[commit-bot: I haz the power, 2016-07-18 09:24:41]

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

[commit-bot: I haz the power, 2016-07-18 09:24:42]

Failed to apply patch for src/compiler/effect-control-linearizer.cc:
While running git apply --index -3 -p1;
  error: patch failed: src/compiler/effect-control-linearizer.cc:1321
  Falling back to three-way merge...
  Applied patch to 'src/compiler/effect-control-linearizer.cc' with conflicts.
  U src/compiler/effect-control-linearizer.cc

Patch:       src/compiler/effect-control-linearizer.cc
Index: src/compiler/effect-control-linearizer.cc
diff --git a/src/compiler/effect-control-linearizer.cc
b/src/compiler/effect-control-linearizer.cc
index
8701c1923b80c8121efcb371af360bf8eb4fd21d..f50650b041904477c14fec53ede6fba8d1c4d920
100644
--- a/src/compiler/effect-control-linearizer.cc
+++ b/src/compiler/effect-control-linearizer.cc
@@ -1308,6 +1308,7 @@ EffectControlLinearizer::LowerCheckedUint32Mod(Node* node,
Node* frame_state,
 EffectControlLinearizer::ValueEffectControl
 EffectControlLinearizer::LowerCheckedInt32Mul(Node* node, Node* frame_state,
                                               Node* effect, Node* control) {
+  CheckForMinusZeroMode mode = CheckMinusZeroModeOf(node->op());
   Node* zero = jsgraph()->Int32Constant(0);
   Node* lhs = node->InputAt(0);
   Node* rhs = node->InputAt(1);
@@ -1321,27 +1322,29 @@ EffectControlLinearizer::LowerCheckedInt32Mul(Node*
node, Node* frame_state,
 
   Node* value = graph()->NewNode(common()->Projection(0), projection, control);
 
-  Node* check_zero = graph()->NewNode(machine()->Word32Equal(), value, zero);
-  Node* branch_zero = graph()->NewNode(common()->Branch(BranchHint::kFalse),
-                                       check_zero, control);
+  if (mode == CheckForMinusZeroMode::kCheckForMinusZero) {
+    Node* check_zero = graph()->NewNode(machine()->Word32Equal(), value, zero);
+    Node* branch_zero = graph()->NewNode(common()->Branch(BranchHint::kFalse),
+                                         check_zero, control);
 
-  Node* if_zero = graph()->NewNode(common()->IfTrue(), branch_zero);
-  Node* e_if_zero = effect;
-  {
-    // We may need to return negative zero.
-    Node* or_inputs = graph()->NewNode(machine()->Word32Or(), lhs, rhs);
-    Node* check_or =
-        graph()->NewNode(machine()->Int32LessThan(), or_inputs, zero);
-    if_zero = e_if_zero = graph()->NewNode(common()->DeoptimizeIf(), check_or,
-                                           frame_state, e_if_zero, if_zero);
-  }
+    Node* if_zero = graph()->NewNode(common()->IfTrue(), branch_zero);
+    Node* e_if_zero = effect;
+    {
+      // We may need to return negative zero.
+      Node* or_inputs = graph()->NewNode(machine()->Word32Or(), lhs, rhs);
+      Node* check_or =
+          graph()->NewNode(machine()->Int32LessThan(), or_inputs, zero);
+      if_zero = e_if_zero = graph()->NewNode(common()->DeoptimizeIf(),
check_or,
+                                             frame_state, e_if_zero, if_zero);
+    }
 
-  Node* if_not_zero = graph()->NewNode(common()->IfFalse(), branch_zero);
-  Node* e_if_not_zero = effect;
+    Node* if_not_zero = graph()->NewNode(common()->IfFalse(), branch_zero);
+    Node* e_if_not_zero = effect;
 
-  control = graph()->NewNode(common()->Merge(2), if_zero, if_not_zero);
-  effect = graph()->NewNode(common()->EffectPhi(2), e_if_zero, e_if_not_zero,
-                            control);
+    control = graph()->NewNode(common()->Merge(2), if_zero, if_not_zero);
+    effect = graph()->NewNode(common()->EffectPhi(2), e_if_zero, e_if_not_zero,
+                              control);
+  }
 
   return ValueEffectControl(value, effect, control);
 }

[commit-bot: I haz the power, 2016-07-18 09:24:44]

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

[commit-bot: I haz the power, 2016-07-18 09:24:45]

CQ bit was unchecked.

[mvstanton, 2016-07-18 11:07:18]

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

[commit-bot: I haz the power, 2016-07-18 11:07:22]

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

[mvstanton, 2016-07-18 11:07:26]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2016-07-18 11:07:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2154073002/#ps60001
(title: "REBASE.")

[commit-bot: I haz the power, 2016-07-18 11:07:32]

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-18 11:34:16]

Description was changed from

==========
[Turbofan]: Eliminate the check for -0 if it's not possible/observable.

In int32 multiplication, if we have a positive integer as input, then we know we
can't produce a -0 answer. The same is true if truncation is applied (x * y |
0). Without this information, we have to rather annoyingly check if the result
of multiplication is 0, then OR the inputs to check for negativity, and possibly
return -0. In TurboFan, we'll deopt in this case.


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

to

==========
[Turbofan]: Eliminate the check for -0 if it's not possible/observable.

In int32 multiplication, if we have a positive integer as input, then we know we
can't produce a -0 answer. The same is true if truncation is applied (x * y |
0). Without this information, we have to rather annoyingly check if the result
of multiplication is 0, then OR the inputs to check for negativity, and possibly
return -0. In TurboFan, we'll deopt in this case.


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

[commit-bot: I haz the power, 2016-07-18 11:34:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-18 11:34:18]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 11:37:30]

Description was changed from

==========
[Turbofan]: Eliminate the check for -0 if it's not possible/observable.

In int32 multiplication, if we have a positive integer as input, then we know we
can't produce a -0 answer. The same is true if truncation is applied (x * y |
0). Without this information, we have to rather annoyingly check if the result
of multiplication is 0, then OR the inputs to check for negativity, and possibly
return -0. In TurboFan, we'll deopt in this case.


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

to

==========
[Turbofan]: Eliminate the check for -0 if it's not possible/observable.

In int32 multiplication, if we have a positive integer as input, then we know we
can't produce a -0 answer. The same is true if truncation is applied (x * y |
0). Without this information, we have to rather annoyingly check if the result
of multiplication is 0, then OR the inputs to check for negativity, and possibly
return -0. In TurboFan, we'll deopt in this case.

BUG=

Committed: https://crrev.com/47aaac644216177be86f93f57c9fb3e1ed23dda3
Cr-Commit-Position: refs/heads/master@{#37831}
==========

[commit-bot: I haz the power, 2016-07-18 11:37:31]

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