Refactor internal switch cases #9802

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.

Sign up for GitHub

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

Jump to bottom

Merged

facebook-github-bot merged 1 commit into pytorch:main from manuelcandales:export-D72248082

Apr 15, 2025

Contributor

manuelcandales commented Apr 1, 2025 •

edited

Loading

There were multiple ET_INTERNAL_SWITCH_CASE_*_TYPES macros that repeated lists of types, for example:
ET_INTERNAL_SWITCH_CASE_INT_TYPES
ET_INTERNAL_SWITCH_CASE_REAL_TYPES
ET_INTERNAL_SWITCH_CASE_FLOAT_TYPES
ET_INTERNAL_SWITCH_CASE_ALL_TYPES
In this PR, we refactor them building from the bottom up, for example reusing INT and FLOAT to build REAL and reusing REAL to build ALL

Differential Revision: D72248082

manuelcandales requested review from JacobSzwejbka, lucylq and swolchok as code owners

April 1, 2025 16:00

pytorch-bot bot commented Apr 1, 2025 •

edited

Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9802

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit b29ea1c with merge base d28670b ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

facebook-github-bot added the CLA Signed label

Contributor

facebook-github-bot commented Apr 1, 2025

This pull request was exported from Phabricator. Differential Revision: D72248082

facebook-github-bot added the fb-exported label

manuelcandales added the release notes: ops & kernels label

swolchok requested changes

View reviewed changes

Contributor

swolchok left a comment

an up-to-date English explanation of the taxonomy of types in a comment is probably in order so people don't have to read through all the macros.

would also like @mergennachin's opinion on this

runtime/core/exec_aten/util/scalar_type_util.h Outdated

Comment on lines 924 to 926

+              #define ET_INTERNAL_SWITCH_CASE_BOOL_TYPES(CTYPE_ALIAS, ...) \
+                ET_INTERNAL_SWITCH_CASE(                                   \
+                    ::executorch::aten::ScalarType::Bool, CTYPE_ALIAS, __VA_ARGS__)

Contributor

swolchok Apr 1, 2025

why does this macro exist? what other types could it contain that aren't exactly ScalarType::Bool?

runtime/core/exec_aten/util/scalar_type_util.h Outdated

+                ET_INTERNAL_SWITCH_CASE(                                              \
+                    ::executorch::aten::ScalarType::UInt64, CTYPE_ALIAS, __VA_ARGS__)
+              #define ET_INTERNAL_SWITCH_CASE_ALLINT_TYPES(CTYPE_ALIAS, ...) \

Contributor

swolchok Apr 1, 2025

why is "ALLINT" one word instead of "ALL_INT"?

Contributor Author

manuelcandales Apr 1, 2025

ALLINT is the name of the set, just like REALH and REALHBF16 are names

runtime/core/exec_aten/util/scalar_type_util.h Outdated

Comment on lines 956 to 962

+              #define ET_INTERNAL_SWITCH_CASE_FLOAT_TYPES(CTYPE_ALIAS, ...)          \
                 ET_INTERNAL_SWITCH_CASE(                                             \
                     ::executorch::aten::ScalarType::Float, CTYPE_ALIAS, __VA_ARGS__) \
                 ET_INTERNAL_SWITCH_CASE(                                             \
                     ::executorch::aten::ScalarType::Double, CTYPE_ALIAS, __VA_ARGS__)
+              #define ET_INTERNAL_SWITCH_CASE_ALLFLOAT_TYPES(CTYPE_ALIAS, ...)      \

Contributor

swolchok Apr 1, 2025

having these both exist is confusing -- "Half and BFloat16 are part of all float types, but they're not float types"?

Contributor Author

manuelcandales Apr 1, 2025

Half and BFloat16 are not part of the FLOAT set today. That's why we have FLOATH and FLOATHBF16.
I am using the prefix "ALL" to denote the set containing all of the types belonging to a given category (INT, FLOAT, REAL, COMPLEX, NUMERIC)

runtime/core/exec_aten/util/scalar_type_util.h Outdated

Comment on lines 969 to 973

+              #define ET_INTERNAL_SWITCH_CASE_REAL_TYPES(CTYPE_ALIAS, ...)  \
+                ET_INTERNAL_SWITCH_CASE_INT_TYPES(CTYPE_ALIAS, __VA_ARGS__) \
+                ET_INTERNAL_SWITCH_CASE_FLOAT_TYPES(CTYPE_ALIAS, __VA_ARGS__)
+              #define ET_INTERNAL_SWITCH_CASE_ALLREAL_TYPES(CTYPE_ALIAS, ...)  \

Contributor

swolchok Apr 1, 2025

same -- if ET_INTERNAL_SWITCH_CASE_REAL_TYPES should not be deprecated, then we need better names that make it clear when you would use each one

runtime/core/exec_aten/util/scalar_type_util.h Outdated

Comment on lines 981 to 987

+              #define ET_INTERNAL_SWITCH_CASE_COMPLEX_TYPES(CTYPE_ALIAS, ...)               \
+                ET_INTERNAL_SWITCH_CASE(                                                    \
+                    ::executorch::aten::ScalarType::ComplexFloat, CTYPE_ALIAS, __VA_ARGS__) \
+                ET_INTERNAL_SWITCH_CASE(                                                    \
+                    ::executorch::aten::ScalarType::ComplexDouble, CTYPE_ALIAS, __VA_ARGS__)
+              #define ET_INTERNAL_SWITCH_CASE_ALLCOMPLEX_TYPES(CTYPE_ALIAS, ...) \

Contributor

swolchok Apr 1, 2025

same concern as before -- confusing, why isn't the old one deprecated, etc.

runtime/core/exec_aten/util/scalar_type_util.h Outdated

Comment on lines 1212 to 1214

               #define ET_SWITCH_REALHBBF16_AND_UINT_TYPES(                  \
                   TYPE, CONTEXT, NAME, CTYPE_ALIAS, ...)                    \
-                ET_INTERNAL_SWITCH(                                         \
-                    TYPE,                                                   \
-                    CONTEXT,                                                \
-                    NAME,                                                   \
-                    ET_INTERNAL_SWITCH_CASE_REAL_TYPES_AND3(                \
-                        Half, Bool, BFloat16, CTYPE_ALIAS, __VA_ARGS__)     \
-                        ET_INTERNAL_SWITCH_CASE(                            \
-                            ::executorch::aten::ScalarType::UInt16,         \
-                            CTYPE_ALIAS,                                    \
-                            __VA_ARGS__)                                    \
-                            ET_INTERNAL_SWITCH_CASE(                        \
-                                ::executorch::aten::ScalarType::UInt32,     \
-                                CTYPE_ALIAS,                                \
-                                __VA_ARGS__)                                \
-                                ET_INTERNAL_SWITCH_CASE(                    \
-                                    ::executorch::aten::ScalarType::UInt64, \
-                                    CTYPE_ALIAS,                            \
-                                    __VA_ARGS__))
+                ET_SWITCH_ALLREALB_TYPES(TYPE, CONTEXT, NAME, CTYPE_ALIAS, __VA_ARGS__)

Contributor

swolchok Apr 1, 2025

this is more easily accomplished as #define ET_SWITCH_REALHBBF16_AND_UINT_TYPES ET_SWITCH_ALLREALB_TYPES (and similarly below)

manuelcandales added a commit to manuelcandales/executorch-1 that referenced this pull request


          Refactor internal switch cases (pytorch#9802)

ed3e10c

Summary:
There were multiple ET_INTERNAL_SWITCH_CASE_*_TYPES macros that repeated lists of types, for example:
`ET_INTERNAL_SWITCH_CASE_INT_TYPES`
`ET_INTERNAL_SWITCH_CASE_REAL_TYPES`
`ET_INTERNAL_SWITCH_CASE_FLOAT_TYPES`
`ET_INTERNAL_SWITCH_CASE_ALL_TYPES`
In this PR, we refactor them building from the bottom up, for example reusing INT and FLOAT to build REAL and reusing REAL to build ALL


Differential Revision: D72248082

manuelcandales force-pushed the export-D72248082 branch from b704763 to ed3e10c Compare

April 1, 2025 18:35

Contributor

facebook-github-bot commented Apr 1, 2025

This pull request was exported from Phabricator. Differential Revision: D72248082

manuelcandales added topic: not user facing and removed release notes: ops & kernels labels

swolchok approved these changes

View reviewed changes

Contributor

swolchok left a comment

seems fine

manuelcandales added a commit to manuelcandales/executorch-1 that referenced this pull request


          Refactor internal switch cases (pytorch#9802)

933fa0e

Summary:
There were multiple ET_INTERNAL_SWITCH_CASE_*_TYPES macros that repeated lists of types, for example:
`ET_INTERNAL_SWITCH_CASE_INT_TYPES`
`ET_INTERNAL_SWITCH_CASE_REAL_TYPES`
`ET_INTERNAL_SWITCH_CASE_FLOAT_TYPES`
`ET_INTERNAL_SWITCH_CASE_ALL_TYPES`
In this PR, we refactor them building from the bottom up, for example reusing INT and FLOAT to build REAL and reusing REAL to build ALL


Reviewed By: swolchok

Differential Revision: D72248082

manuelcandales force-pushed the export-D72248082 branch from ed3e10c to 933fa0e Compare

April 4, 2025 01:52

Contributor

facebook-github-bot commented Apr 4, 2025

This pull request was exported from Phabricator. Differential Revision: D72248082

github-actions bot mentioned this pull request

Weekly pr metrics report - 2025-04-01..2025-04-07 wdvr/pytorch#28

Open


          Refactor internal switch cases (pytorch#9802)

b29ea1c

Summary:
There were multiple ET_INTERNAL_SWITCH_CASE_*_TYPES macros that repeated lists of types, for example:
`ET_INTERNAL_SWITCH_CASE_INT_TYPES`
`ET_INTERNAL_SWITCH_CASE_REAL_TYPES`
`ET_INTERNAL_SWITCH_CASE_FLOAT_TYPES`
`ET_INTERNAL_SWITCH_CASE_ALL_TYPES`
In this PR, we refactor them building from the bottom up, for example reusing INT and FLOAT to build REAL and reusing REAL to build ALL


Reviewed By: swolchok

Differential Revision: D72248082

manuelcandales force-pushed the export-D72248082 branch from 933fa0e to b29ea1c Compare

April 14, 2025 18:17

Contributor

facebook-github-bot commented Apr 14, 2025

This pull request was exported from Phabricator. Differential Revision: D72248082

facebook-github-bot merged commit 6d1caca into pytorch:main

83 of 84 checks passed

github-actions bot mentioned this pull request

Weekly pr metrics report - 2025-04-01..2025-04-07 wdvr/pytorch#30

Open

keyprocedure pushed a commit to keyprocedure/executorch that referenced this pull request


          Refactor internal switch cases

39fbde9

Differential Revision: D72248082

Pull Request resolved: pytorch#9802

github-actions bot mentioned this pull request

Weekly pr metrics report - 2025-04-01..2025-04-07 wdvr/pytorch#35

Open

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

Labels

CLA Signed fb-exported topic: not user facing