-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add regression test for broken non commutative multiplication (#9175) #9218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Current test output is:
object(NonCommutativeMultiplication)#1 (1) {
["val":"NonCommutativeMultiplication":private]=>
int(10)
}
object(NonCommutativeMultiplication)#2 (1) {
["val":"NonCommutativeMultiplication":private]=>
int(50)
}
object(NonCommutativeMultiplication)#3 (1) {
["val":"NonCommutativeMultiplication":private]=>
int(100)
}
object(NonCommutativeMultiplication)#5 (1) {
["val":"NonCommutativeMultiplication":private]=>
int(30)
}
int(40)
object(NonCommutativeMultiplication)#4 (1) {
["val":"NonCommutativeMultiplication":private]=>
int(-40)
}
int(-140)
object(NonCommutativeMultiplication)#5 (1) {
["val":"NonCommutativeMultiplication":private]=>
int(140)
}
int(-60)
object(NonCommutativeMultiplication)#4 (1) {
["val":"NonCommutativeMultiplication":private]=>
int(60)
}
(削除) Which I don't even understand how it got to those results. (削除ここまで)
It reorders the operation from:
((100 - 50) - 10)
to
(10 - (100 - 50))
336e6e2 to
f4a123b
Compare
Okay I've got a fix, which is to remove the SPEC(COMMUTATIVE) marker in the VM.
Not sure how much this will affect performances, and or if there is another way to handle it.
As it might not be when an internal object overloads the multiplication operation
f4a123b to
e0350f4
Compare
This shouldn't affect performance directly. This will slightly increase VM size and reduce code locality.
I'm not sure if simple removing the "commutativity" flag is enough. May be optimizer and JIT also relay on multiplication commutativity (I didn't check). Anyway, I don't like the idea of changing engine in favour of third-party extensions.
At first, we allow to break operators commutativity, then we will need a user defined operators order, etc
This should add a test to catch the issue pointed out by #9175
Currently XFAIL as well it is broken.
Not sure this is the best way to write an internal object so reviews on this aspect would be appreciated.