Skip to content

Conversation

@lara-hdr
Copy link
Contributor

@lara-hdr lara-hdr commented Oct 17, 2019

Support exporting left/right bitshifts to ONNX for all opset versions.

ONNX has a bitshift operator in opset 11, but it only supports unsigned ints, so it can't be used in PyTorch (since only uint8 is the only uint type).

Copy link

@spandantiwari spandantiwari left a comment

Choose a reason for hiding this comment

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

Could you please confirm if onnx::bitshift can be used here or not.

@lara-hdr
Copy link
Contributor Author

@pytorchbot rebase this please

@neginraoof
Copy link
Contributor

CI failure is TestONNXRuntime_opset11::test_bitshift_uint8 FAILED

@lara-hdr
Copy link
Contributor Author

lara-hdr commented Nov 6, 2019

thank you @neginraoof, I had forgotten about this one. I updated the ORT version for the test.

@lara-hdr lara-hdr requested a review from BowenBao November 6, 2019 22:07
@neginraoof
Copy link
Contributor

Thanks. Looks good.

@lara-hdr
Copy link
Contributor Author

@pytorchbot rebase this please

Copy link
Collaborator

@BowenBao BowenBao left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

Copy link

@spandantiwari spandantiwari left a comment

Choose a reason for hiding this comment

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

LGTM. Please address a single comment that I have.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@lara-hdr
Copy link
Contributor Author

Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

LGTM

@facebook-github-bot
Copy link
Contributor

@houseroad merged this pull request in 45024e7.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants