If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

inconsistent setting of isCommutative flag

NEW
Assigned to

Status

()

Core
JavaScript Engine
--
enhancement
4 years ago
3 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 781718 [details] [diff] [review]
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.
Attachment #781718 - Flags: review?(sstangl)
Unspecialized add probably shouldn't be commutative, since it can be an addition of strings, which is *not* commutative.
(Assignee)

Comment 2

4 years ago
Aha, thanks for showing me what I missed. I'll revise the patch.
(Assignee)

Comment 3

4 years ago
Created attachment 781765 [details] [diff] [review]
commutative.patch

This drops the MAdd changes.
Attachment #781718 - Attachment is obsolete: true
Attachment #781718 - Flags: review?(sstangl)
Attachment #781765 - Flags: review?(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.
Attachment #781765 - Flags: review?(sstangl)
(Assignee)

Comment 5

4 years ago
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?
(Assignee)

Comment 6

4 years ago
(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.
QA Contact: sunfish
(Assignee)

Updated

4 years ago
Assignee: general → sunfish
QA Contact: sunfish
You need to log in before you can comment on or make changes to this bug.