Skip to content
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

[quant] AT_DISPATCH_FLOATING_AND_QINT_TYPES #48050

Closed
wants to merge 2 commits into from

Conversation

z-a-f
Copy link
Contributor

@z-a-f z-a-f commented Nov 16, 2020

Stack from ghstack:

The switch does not require testing -- if need to confirm if it works right, you can run the tests for the reflection padding: python test/test_quantization.py TestPadding

Differential Revision: D25004159

Copy link
Contributor

@vkuzo vkuzo left a comment

Choose a reason for hiding this comment

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

lg, can we add a test plan to the diff summary

@z-a-f
Copy link
Contributor Author

z-a-f commented Nov 16, 2020

lg, can we add a test plan to the diff summary

What kind of test for the switch are you looking for?

@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #48050 (5277246) into gh/z-a-f/87/base (c334fbc) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@                 Coverage Diff                  @@
##           gh/z-a-f/87/base   #48050      +/-   ##
====================================================
- Coverage             81.30%   81.30%   -0.01%     
====================================================
  Files                  1839     1839              
  Lines                198452   198452              
====================================================
- Hits                 161353   161345       -8     
- Misses                37099    37107       +8     

@@ -280,6 +280,25 @@ inline void deprecated_AT_DISPATCH_ALL_TYPES_AND_HALF_AND_COMPLEX() {}
} \
}()

#define AT_DISPATCH_FLOATING_AND_QINT_TYPES(TYPE, NAME, ...) \
Copy link
Contributor

Choose a reason for hiding this comment

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

oh I thought we can compose...would be even better if we can compose AT_DISPATCH_FLOATING_TYPES and AT_DISPATCH_QINT_TYPES somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Composition would require rewriting the dispatch switches to avoid the the default case.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we just define a macro for 3 AT_QINT_PRIVATE_CASE_TYPE and 2 AT_PRIVATE_CASE_TYPE ?
e.g.

#define QINT_CASES(__VA_ARGS__) AT_QINT_PRIVATE_CASE_TYPE(...) \
   AT_QINT_PRIVATE_CASE_TYPE(...) \
   AT_QINT_PRIVATE_CASE_TYPE(...)

Comment on lines +291 to +296
AT_QINT_PRIVATE_CASE_TYPE( \
at::kQInt8, at::qint8, at::kChar, int8_t, __VA_ARGS__) \
AT_QINT_PRIVATE_CASE_TYPE( \
at::kQUInt8, at::quint8, at::kByte, uint8_t, __VA_ARGS__) \
AT_QINT_PRIVATE_CASE_TYPE( \
at::kQInt32, at::qint32, at::kInt, int, __VA_ARGS__) \
Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. we might be able to compose if we move these cases to a macro, and also the float cases to another macro

@github-actions
Copy link

## Stale alert! Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
Stale pull requests will automatically be closed 30 days after being marked Stale

@github-actions github-actions bot added the Stale label Jan 28, 2021
@github-actions github-actions bot closed this Apr 13, 2022
@facebook-github-bot facebook-github-bot deleted the gh/z-a-f/87/head branch May 13, 2022 14:20
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.

None yet

4 participants