Skip to content

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Aug 1, 2022

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.

@Girgias
Copy link
Member Author

Girgias commented Aug 1, 2022

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))

@Girgias Girgias requested a review from kocsismate August 2, 2022 14:49
@Girgias Girgias force-pushed the non-commutative-mul-test branch from 336e6e2 to f4a123b Compare August 2, 2022 16:02
@Girgias Girgias requested a review from dstogov August 2, 2022 16:03
@Girgias
Copy link
Member Author

Girgias commented Aug 2, 2022

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.

@Girgias Girgias force-pushed the non-commutative-mul-test branch from f4a123b to e0350f4 Compare August 4, 2022 12:43
@dstogov
Copy link
Member

dstogov commented Aug 8, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants