Simplify endianness check using std::endian#186854
Conversation
`std::endian` is available since C++20 pytorch#176662
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/186854
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New Failures, 2 Unrelated FailuresAs of commit 70b9da7 with merge base b9a2479 ( NEW FAILURES - The following jobs have failed:
UNSTABLE - The following jobs are marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
| @@ -107,8 +108,11 @@ static uint64_t decodeUInt64ByteSwapped(const uint8_t* data) { | |||
| namespace torch::utils { | |||
|
|
|||
| THPByteOrder THP_nativeByteOrder() { | |||
There was a problem hiding this comment.
Done. BTW do you why we did not define it using the preprocessor macros used in the header?
THPByteOrder THP_nativeByteOrder() {
#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
return THP_LITTLE_ENDIAN;
#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
return THP_BIG_ENDIAN;
#else
static_assert(false, ...);
#endif
}I ask because I'm trying to understand if there was a reason behind making this a runtime vs compile time check
There was a problem hiding this comment.
I don't know its history, but you can simplify
There was a problem hiding this comment.
Is it possible for the library to be compiled on a machine of one endian and run on a machine of another?
| THPByteOrder THP_nativeByteOrder() { | ||
| uint32_t x = 1; | ||
| return *(uint8_t*)&x ? THP_LITTLE_ENDIAN : THP_BIG_ENDIAN; | ||
| constexpr THPByteOrder THP_nativeByteOrder() { |
There was a problem hiding this comment.
Think this has to be header only now that it's constexpr right?
There was a problem hiding this comment.
Oh right, it won't be usable at compile time unless it is in the header. However, I don't want to put C++20 stuff in header since it ends up causing problems with vllm. Should I just remove the constexpr for now?
There was a problem hiding this comment.
Unfortunately more and more CPP20 stuff will be in headers soon so I recommend VLLM upgrades soon.
There was a problem hiding this comment.
How is it getting past the hard CPP20 version guard anyway?
There was a problem hiding this comment.
They use PyTorch libs and headers. libs are compiled with C++20 but headers that make use of C++20 features and get included in their build cause issues IIUC.
This reverts commit 624143d.
std::endianis available since C++20 #176662