Closed
Bug 898450
Opened 11 years ago
Closed 3 months ago
inconsistent setting of isCommutative flag
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
INCOMPLETE
People
(Reporter: sunfish, Unassigned)
Details
Attachments
(1 file, 1 obsolete file)
5.60 KB,
patch
|
Details | Diff | Splinter Review |
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)
Comment 1•11 years ago
|
||
Unspecialized add probably shouldn't be commutative, since it can be an addition of strings, which is *not* commutative.
Reporter | ||
Comment 2•11 years ago
|
||
Aha, thanks for showing me what I missed. I'll revise the patch.
Reporter | ||
Comment 3•11 years ago
|
||
This drops the MAdd changes.
Attachment #781718 -
Attachment is obsolete: true
Attachment #781718 -
Flags: review?(sstangl)
Attachment #781765 -
Flags: review?(sstangl)
Comment 4•11 years ago
|
||
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)
Reporter | ||
Comment 5•11 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?
Reporter | ||
Comment 6•11 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
Reporter | ||
Updated•11 years ago
|
Assignee: general → sunfish
QA Contact: sunfish
Comment 7•2 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.
Assignee: sunfish → nobody
Updated•2 years ago
|
Severity: normal → S3
Updated•3 months ago
|
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•