Msvc ops changes #15226
Msvc ops changes #15226
Conversation
JacobSzwejbka
requested review from
kirklandsign,
larryliu0820,
lucylq,
manuelcandales and
swolchok
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/15226
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 1 New Failure, 1 Unrelated FailureAs of commit bc71d1c with merge base 173b046 ( NEW FAILURE - The following job has failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
bot
added
the
CLA Signed
This PR needs a
|
| _to_impl<CTYPE_IN, CTYPE_OUT>(self, out); | ||
| }); | ||
| }); | ||
| ET_SWITCH_COMPLEX_TYPES( |
There was a problem hiding this comment.
oh lost the H on complexh
JacobSzwejbka
requested review from
jackzhxng and
mergennachin
as code owners
| static_assert( | ||
| (std::is_same_v<Args, std::pair<const Tensor*, SupportedTensorDtypes>> && | ||
| ...)); | ||
| constexpr auto kNumInputs = sizeof...(inputs); |
There was a problem hiding this comment.
I tried a couple things to make msvc recognize kNumInputs here as being constant at compile time but it refused. Weirdly it does still recognize sizeof...(inputs) so I can just update the usage
| ET_KERNEL_CHECK( | ||
| ctx, check_bmm_out_args(self, mat2, out), InvalidArgument, out); | ||
|
|
||
| constexpr auto name = "bmm.out"; |
There was a problem hiding this comment.
I don't suppose we are lucky enough that static constexpr auto name = "bmm.out"; would fix it?
| set(_common_compile_options | ||
| $<$<CXX_COMPILER_ID:MSVC>:/wd4996> | ||
| $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wno-deprecated-declarations> | ||
| ) |
There was a problem hiding this comment.
I don't have a strong preference here, just noting that the following is also valid:
if (MSVC)
set(_common_compile_options /wd4996)
else()
set(_common_compile_options -Wno-deprecated-declarations)
endif()
I like that it avoids the ugly $<$<NOT:$<X>>>, but I don't like that it duplicates the variable name. up to your sense of taste.
| static_assert( | ||
| (std::is_same_v<Args, std::pair<const Tensor*, SupportedTensorDtypes>> && | ||
| ...)); | ||
| constexpr auto kNumInputs = sizeof...(inputs); |
There was a problem hiding this comment.
the strings are understandable (I had to be pretty careful just to get clang to accept them IIRC), but this one is pretty disappointing and also seems to work on godbolt. have you tried generating a minimal example?
| const auto cmp = largest ? elem_greater : elem_less; | ||
| if (use_partial_sort) { | ||
| std::partial_sort(queue, queue + k, queue + dim_size, cmp); | ||
| if (largest) { |
There was a problem hiding this comment.
what's the problem with this one? these don't seem to be constexpr variables at all. error message might be helpful
There was a problem hiding this comment.
Piping lambdas into the ternary was having issues. It couldnt resolve that they had the same return type
There was a problem hiding this comment.
Oh, you should be able to fix that by explicitly using the appropriate function pointer type instead of auto
2 participants
Summary
MSVC really doesn't like the constexpr variables that I've removed in this PR. It was complaining about them not actually being static at compile time. Searching around suggests that its the act of putting them in a variable that breaks it and if I just place the value directly in all the used locations it should be fine. I tried that and it works.
The other changes are fixing up some compiler flags, adding a missing include, fixing an interaction between lambdas the ternary operator and auto, and removing unneeded statement expressions
Test plan
current ci to verify no regression on linux/mac. Manual testing of msvc will add ci soon