-
Notifications
You must be signed in to change notification settings - Fork 320
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
Support signed int divisor in onnx.Mod op #2471
Support signed int divisor in onnx.Mod op #2471
Conversation
Signed-off-by: Yasushi Negishi <[email protected]>
Signed-off-by: Yasushi Negishi <[email protected]>
Should we use llvm srem? |
I investigated definitions and behavior of llvm.srem, and arith.remsi (https://mlir.llvm.org/docs/Dialects/ArithOps/#arithremsi-arithremsiop), that is converted into llvm.srem. According to the investigation, these operations support signed integers, but they cannot generate right answers when the divisor is minus. If we simply use arith.remsi for onnx.Mod, the backend tests ("test_mod_mixed_sign_int64_cpu", "test_mod_mixed_sign_int32_cpu") fail as follows.
Accordingly, it seems that additional logic in this PR is necessary to generate right answers for the divisor-is-minus cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
// rem = rem - divisor // make rem minus | ||
// if (divisor < 0) | ||
// rem = rem + divisor; // make rem minus | ||
// return rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is little tricky. To understand it, I list out the case and result according to your code (NOT the comment):
- dividend > 0 and divisor >0, rem unchanged
- dividend < 0 and divisor <0, rem = (rem+divisor) + divisor = rem + 2*divisor
- dividend < 0 and divisor > 0, rem = rem + divisor = rem + |divisor|
- dividend > 0 and divisor < 0, rem = rem+divisor = rem - |divisor|
Is the second case correct? Please fix the difference of the comment and code.
Another question: do we need to check eq to zero again at line 1156? If the original rem is not zero, it will be zero after the added calculation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chentong319 I updated the explanation and code for the part.
Thank you for useful comments. Can you kindly check the comments and code again?
Signed-off-by: Yasushi Negishi <[email protected]>
Signed-off-by: Yasushi Negishi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Jenkins Linux amd64 Build #12613 [push] Support signed int divis... started at 01:29 |
Jenkins Linux s390x Build #12625 [push] Support signed int divis... started at 02:29 |
Jenkins Linux ppc64le Build #11618 [push] Support signed int divis... started at 02:38 |
Jenkins Linux amd64 Build #12613 [push] Support signed int divis... passed after 1 hr 0 min |
Jenkins Linux s390x Build #12625 [push] Support signed int divis... passed after 1 hr 24 min |
Jenkins Linux ppc64le Build #11618 [push] Support signed int divis... passed after 1 hr 46 min |
This PR supports signed int divisor in onnx.Mod op as well as unsigned int divisor. This PR includes the conversion pass, its lit and backend tests. The following comments explain difference between return values of math.rem and onnx.Mod op.