898450 - inconsistent setting of isCommutative flag

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: commutative.patch

MBinaryBitwiseInstruction sets isCommutative unconditionally for Int32 operands. Unfortunately, MShiftInstruction inherits from MBinaryBitwiseInstruciton, so shifts are currently being marked as commutative (though this happens to be harmless at the moment, due to the way the shifts' congruentTo functions are implemented).

Also, several operators are not being marked as commutative which seemingly could be: min, max, ==, !=, and also unspecialized add and multiply, unless there's something I'm missing.

The attached patch fixes both. Math.min(a, b) and Math.min(b, a) are considered congruent by ValueNumbering, for example.

Comment 1

[Marty Rosenberg [:mjrosenb]]

Unspecialized add probably shouldn't be commutative, since it can be an addition of strings, which is *not* commutative.

Comment 2

[Dan Gohman [:sunfish]]

Aha, thanks for showing me what I missed. I'll revise the patch.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: commutative.patch

This drops the MAdd changes.

Comment 4

[Sean Stangl [:sstangl]]

Comment on attachment 781765 [details] [diff] [review]
commutative.patch

Review of attachment 781765 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/MIR.h
@@ +1922,5 @@
>      {
>          setResultType(MIRType_Boolean);
>          setMovable();
> +        if (jsop == JSOP_EQ || jsop == JSOP_NE || jsop == JSOP_STRICTEQ || jsop == JSOP_STRICTNE)
> +            setCommutative();

This appears to be safe. I was worried that we could emit an LCompareVM with two objects that have observable side-effects on toValue(), but such a function is only called when comparing with a primitive, in which case the positioning of the object is irrelevant.

@@ +2787,5 @@
>  class MBitAnd : public MBinaryBitwiseInstruction
>  {
>      MBitAnd(MDefinition *left, MDefinition *right)
>        : MBinaryBitwiseInstruction(left, right)
> +    { 

nit: trailing whitespace

@@ +2788,5 @@
>  {
>      MBitAnd(MDefinition *left, MDefinition *right)
>        : MBinaryBitwiseInstruction(left, right)
> +    { 
> +        setCommutative();

Note that x&y (and x|y and x^y) are not commutative in JavaScript, given objects with observable side effects on toValue():

>> var x = {toValue: function(){print("foo");}};
>> var y = {toValue: function(){print("bar");}};
>>
>> x&y // prints "foo\nbar\n"
>> y&x // prints "bar\nfoo\n"

MBitAnd is saved by its BitwisePolicy, which boxes objects and inserts MTruncateToInt32 instructions -- but the ordering of the MTruncateToInt32 operations matters for preserving execution order. Even still, this is safe, because ApplyTypeInformation() executes before GVN, and because BitwisePolicy::adjustInputs() inserts MTruncateToInt32 instructions in the right order.

@@ +3438,5 @@
>  
>          if (type != MIRType_Value)
>              specialization_ = type;
>          setResultType(type);
> +        setCommutative();

This is invalid: if inputs are boxed Objects, then order matters, for the same reason given above.

Comment 5

[Dan Gohman [:sunfish]]

I'm confused. Assuming you meant valueOf in place of toValue, I ran this code:

  var x = {valueOf: function(){print("foo");}};
  var y = {valueOf: function(){print("bar");}};

  print(x*y);
  print(y*x);

with and without this patch:

diff --git a/js/src/ion/MIR.h b/js/src/ion/MIR.h
--- a/js/src/ion/MIR.h
+++ b/js/src/ion/MIR.h
@@ -3424,7 +3424,7 @@ class MMul : public MBinaryArithInstruct
     Mode mode_;
 
     MMul(MDefinition *left, MDefinition *right, MIRType type, Mode mode)
-      : MBinaryArithInstruction(left, right),
+      : MBinaryArithInstruction(right, left),
         canBeNegativeZero_(true),
         mode_(mode)
     {

and got the same output both ways:

foo
bar
NaN
bar
foo
NaN

Is there a way I can observe the effects of operand ordering?

Comment 6

[Dan Gohman [:sunfish]]

(In reply to Dan Gohman [:sunfish] from comment #5)
> I'm confused. Assuming you meant valueOf in place of toValue, I ran this
> code:
> 
>   var x = {valueOf: function(){print("foo");}};
>   var y = {valueOf: function(){print("bar");}};
> 
>   print(x*y);
>   print(y*x);

And the answer is, code executed once isn't compiled by ion even with --ion-eager. Putting this code in a function and calling it twice activates ion, and the above patch does change the output.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.