Skip to content

Bitwise ops improvements#77621

Closed
zasdfgbnm wants to merge 4 commits intomasterfrom
shift-fix-scalar
Closed

Bitwise ops improvements#77621
zasdfgbnm wants to merge 4 commits intomasterfrom
shift-fix-scalar

Conversation

@zasdfgbnm
Copy link
Collaborator

@zasdfgbnm zasdfgbnm commented May 17, 2022 •
edited
Loading

  • Bitwise shift remove floating point support
  • Bitwise and, or, xor add (scalar, tensor) overload
  • Use test_ops.py to test these ops, including error cases

@zasdfgbnm zasdfgbnm requested review from mruberry and ngimel as code owners May 17, 2022 01:35
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 17, 2022 •
edited
Loading

🔗 Helpful links

✅ No Failures (0 Pending)

As of commit 032f324 (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@zasdfgbnm zasdfgbnm mentioned this pull request May 17, 2022
Closed
ngimel
ngimel reviewed May 17, 2022
View reviewed changes
test/test_binary_ufuncs.py Outdated
)

@dtypes(torch.uint8, torch.int8, torch.int16, torch.int32, torch.int64)
@dtypes(torch.uint8, torch.int8, torch.int16, torch.int32, torch.int64, torch.float)
Copy link
Collaborator

@ngimel ngimel May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a todo to port this test to error_inputs in OpInfos?

Copy link
Collaborator Author

@zasdfgbnm zasdfgbnm May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this PR and removed this test

Copy link
Collaborator

@ngimel ngimel May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@ngimel
Copy link
Collaborator

ngimel commented May 17, 2022

QUantization, seriously?

@zasdfgbnm zasdfgbnm requested a review from bdhirsh as a code owner May 17, 2022 05:45
@zasdfgbnm zasdfgbnm changed the title Bitwise shift remove support for non-integral scalar Bitwise ops improvements May 17, 2022
@lezcano
Copy link
Collaborator

lezcano commented May 17, 2022

Could you also update the kernels (there is a dedicated path that implements this) and the docs?
I just found last week about this weird behaviour and it is even documented.

@ngimel
Copy link
Collaborator

ngimel commented May 17, 2022

onnx failure looks real (they are also probably using fp shift scalar)

@zasdfgbnm
Copy link
Collaborator Author

zasdfgbnm commented May 17, 2022

@lezcano which kernel are you referring to? Are you talking about the kernel that I already removed from my previous PR? https://github.com/pytorch/pytorch/pull/77146/files#diff-0fd0c99904c62a1a7e46b08568d047c4e9ab153b529a8c6a2317ac6a3726bdff

@lezcano
Copy link
Collaborator

lezcano commented May 17, 2022

oops. Yes, I was referring to that kernel, thanks!
Also, note that, at the moment, for a << b, if b is greater than sizeof(a) this is UB. We should use a ternary operator there really. Same in CUDA.
For the r-shift, I'm not sure whether a >> b is well-defined when b > sizeof(a).

@zasdfgbnm
Copy link
Collaborator Author

zasdfgbnm commented May 17, 2022

I think starting from C++20, they are no longer undefined https://en.cppreference.com/w/cpp/language/operator_arithmetic. I guess PyTorch should not worry about whatever C++ does not provide in this case? When C++20 is adopted, this issue will be automatically resolved.

@ngimel
Copy link
Collaborator

ngimel commented May 17, 2022

Yeah, we are doing whatever c++ does (and that includes implementation-defined logical/binary rshift for signed args)

@ngimel
Copy link
Collaborator

ngimel commented May 17, 2022

@pytorchbot merge this

@github-actions
Copy link
Contributor

github-actions bot commented May 17, 2022

Hey @zasdfgbnm.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

@zasdfgbnm zasdfgbnm deleted the shift-fix-scalar branch May 17, 2022 23:18
@zasdfgbnm zasdfgbnm added the topic: bc breaking topic category label May 18, 2022
@zasdfgbnm
Copy link
Collaborator Author

zasdfgbnm commented May 18, 2022

Adding topic:bc_breaking label as this is a follow up of #77146 that further removes floating-point support of bitwise shift ops on scalar inputs.

facebook-github-bot pushed a commit that referenced this pull request May 18, 2022
@zasdfgbnm @facebook-github-bot
Bitwise ops improvements (#77621) (#77621)
d77b221 
Summary:
- Bitwise shift remove floating point support
- Bitwise and, or, xor add (scalar, tensor) overload
- Use `test_ops.py` to test these ops, including error cases

Pull Request resolved: #77621
Approved by: https://github.com/ngimel

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/f274558018ff3beefe6282d59bea0350a632484f

Reviewed By: b0noI

Differential Revision: D36466067

Pulled By: b0noI

fbshipit-source-id: 66e08e665f6f8d23c622fb5f683abc5de9ac2884
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ngimel ngimel ngimel approved these changes

@mruberry mruberry Awaiting requested review from mruberry

@bdhirsh bdhirsh Awaiting requested review from bdhirsh

@BowenBao BowenBao Awaiting requested review from BowenBao

@shubhambhokare1 shubhambhokare1 Awaiting requested review from shubhambhokare1

Assignees

No one assigned

Labels

cla signed Merged open source topic: bc breaking topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@zasdfgbnm @facebook-github-bot @ngimel @lezcano @pytorchbot @pytorchmergebot