This repository was archived by the owner on Apr 23, 2021. It is now read-only.
[spirv] Add binary arithmetic operations #2. #56
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add binary operations such as: OpUdiv, OpSDiv, OpUMod, OpSRem, OpSMod.
Contributor
|
@denis0x0D: The split sounds reasonable to me! |
Contributor
|
@denis0x0D : Splitting it sounds like a good idea. |
Contributor
Author
|
@antiagainst @MaheshRavishankar thanks a lot for review! |
Contributor
|
No, I am not seeing that failure. Let me try building this again and check it. |
Contributor
|
I am not seeing this failure. @antiagainst : do you see this failure? @denis0x0D : Just to confirm, you are syncing to ToT? |
Contributor
|
I wasn't able to reproduce the error either (ninja 1.8.2, GCC 7.4.0). What's your environment? |
Contributor
Author
|
@MaheshRavishankar thanks for the answer, I'm on the master branch now, here is my backtrace: it migth be something wrong with my env, but the strange thing is that all tests passes right before this commit. Thanks. |
Contributor
|
thanks for the trace. So this segfaulting? Since the segfault seems to be happening in the autogenerated file, do you mind posting some of the code around the segfaulting line number? |
Contributor
Author
|
@antiagainst @MaheshRavishankar I'm building with Unix like MakeFile, not ninja, so, might be this is a problem. sorry for bothering you. |
Contributor
Author
|
@MaheshRavishankar it points to the line #129 on the fail |
Contributor
|
Not a bother at all. If there is an issue, would rather fix it, so this is good to know. |
Contributor
|
Dont think it is an issue with Ninja. What is your compiler? |
tensorflow-copybara
pushed a commit
to tensorflow/tensorflow
that referenced
this pull request
Jul 31, 2019
Add binary operations such as: OpUdiv, OpSDiv, OpUMod, OpSRem, OpSMod. Closes #56 COPYBARA_INTEGRATE_REVIEW=tensorflow/mlir#56 from denis0x0D:sandbox/bin_ops_int 4959325a693b4658b978a8b97f79b8237eb39764 PiperOrigin-RevId: 260961681
Contributor
Author
|
@MaheshRavishankar it fails on ninja too, I'm building with gcc 8.2.0 the stack-frame #14 points but if noone face it and CI tests passed, this is something wrong with my env. |
Contributor
|
I think there might be an issue. the |
Contributor
|
That's definitely the issue. Have a fix in flight. Thanks for bringing it to our attention. We use gcc 7.4.2. and that doesn't seem to result in a segfault. |
Contributor
Author
|
@MaheshRavishankar thanks for looking on this! |
Contributor
|
Thanks @MaheshRavishankar for 32b81a8, which should have it fixed. @denis0x0D please try it out. |
Contributor
Author
|
@antiagainst @MaheshRavishankar works for me, thanks! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adds other binary arithmetic operations such as: OpUdiv, OpSDiv, OpUMod, OpSRem, OpSMod.
@antiagainst can you please take a look?
BTW, small question about other spirv operations such as logical operations.
In case of spirv all logical binary operations return scalar boolean or vector of boolean, would it be valid to split binary operation to arithmetic and logical like this: