[Support] Redefine endianness::native

[kazutakahirata]

We should eventually migrate llvm::support::endianness to std::endian
when C++20 is available for the codebase.

This patch prepares our codebase for that future by removing the
assumption that native is a unique value different from both big and
little. Note that in C++20, native is equal to either big or little
on typical machines, where the endianness is the same for all scalar
types.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Changes

We should eventually migrate llvm::support::endianness to std::endian
when C++20 is available for the codebase.

This patch prepares our codebase for that future by removing the
assumption that native is a unique value different from both big and
little. Note that in C++20, native is equal to either big or little
on typical machines, where the endianness is the same for all scalar
types.

---

Full diff: https://github.com/llvm/llvm-project/pull/67876.diff

3 Files Affected:

• (modified) llvm/include/llvm/Support/Endian.h (+6-2)
• (modified) llvm/include/llvm/Support/HashBuilder.h (+1-11)
• (modified) llvm/lib/ExecutionEngine/JITLink/aarch32.cpp (-2)

diff --git a/llvm/include/llvm/Support/Endian.h b/llvm/include/llvm/Support/Endian.h
index d407ed112767115..9bfe46022859c9e 100644
--- a/llvm/include/llvm/Support/Endian.h
+++ b/llvm/include/llvm/Support/Endian.h
@@ -24,7 +24,11 @@
 namespace llvm {
 namespace support {
 
-enum endianness {big, little, native};
+enum endianness {
+  big,
+  little,
+  native = llvm::sys::IsBigEndianHost ? big : little
+};
 
 // These are named values for common alignments.
 enum {aligned = 0, unaligned = 1};
@@ -47,7 +51,7 @@ constexpr endianness system_endianness() {
 
 template <typename value_type>
 [[nodiscard]] inline value_type byte_swap(value_type value, endianness endian) {
-  if ((endian != native) && (endian != system_endianness()))
+  if (endian != native)
     sys::swapByteOrder(value);
   return value;
 }
diff --git a/llvm/include/llvm/Support/HashBuilder.h b/llvm/include/llvm/Support/HashBuilder.h
index 04a7b2e7dc8ab26..c3fa011b6aa7e9c 100644
--- a/llvm/include/llvm/Support/HashBuilder.h
+++ b/llvm/include/llvm/Support/HashBuilder.h
@@ -86,15 +86,8 @@ template <typename HasherT> class HashBuilderBase {
 };
 
 /// Implementation of the `HashBuilder` interface.
-///
-/// `support::endianness::native` is not supported. `HashBuilder` is
-/// expected to canonicalize `support::endianness::native` to one of
-/// `support::endianness::big` or `support::endianness::little`.
 template <typename HasherT, support::endianness Endianness>
 class HashBuilderImpl : public HashBuilderBase<HasherT> {
-  static_assert(Endianness != support::endianness::native,
-                "HashBuilder should canonicalize endianness");
-
 public:
   explicit HashBuilderImpl(HasherT &Hasher)
       : HashBuilderBase<HasherT>(Hasher) {}
@@ -395,10 +388,7 @@ class HashBuilderImpl : public HashBuilderBase<HasherT> {
 /// Specifiying a non-`native` `Endianness` template parameter allows to compute
 /// stable hash across platforms with different endianness.
 template <class HasherT, support::endianness Endianness>
-using HashBuilder =
-    HashBuilderImpl<HasherT, (Endianness == support::endianness::native
-                                  ? support::endian::system_endianness()
-                                  : Endianness)>;
+using HashBuilder = HashBuilderImpl<HasherT, Endianness>;
 
 namespace hashbuilder_detail {
 class HashCodeHasher {
diff --git a/llvm/lib/ExecutionEngine/JITLink/aarch32.cpp b/llvm/lib/ExecutionEngine/JITLink/aarch32.cpp
index 37ced88169b0ac4..124955236cf96a9 100644
--- a/llvm/lib/ExecutionEngine/JITLink/aarch32.cpp
+++ b/llvm/lib/ExecutionEngine/JITLink/aarch32.cpp
@@ -306,7 +306,6 @@ void writeImmediate(WritableArmRelocation &R, uint32_t Imm) {
 
 Expected<int64_t> readAddendData(LinkGraph &G, Block &B, const Edge &E) {
   support::endianness Endian = G.getEndianness();
-  assert(Endian != support::native && "Declare as little or big explicitly");
 
   Edge::Kind Kind = E.getKind();
   const char *BlockWorkingMem = B.getContent().data();
@@ -404,7 +403,6 @@ Error applyFixupData(LinkGraph &G, Block &B, const Edge &E) {
   char *FixupPtr = BlockWorkingMem + E.getOffset();
 
   auto Write32 = [FixupPtr, Endian = G.getEndianness()](int64_t Value) {
-    assert(Endian != native && "Must be explicit: little or big");
     assert(isInt<32>(Value) && "Must be in signed 32-bit range");
     uint32_t Imm = static_cast<int32_t>(Value);
     if (LLVM_LIKELY(Endian == little))

[zero9178]

LGTM! I do not see the point of endianness::native being a distinct value either. Please wait a few days (probably best until after the weekend) before landing this to give others a chance to review as well.

Are there any plans to also remove system_endianness()? It seems entirely equivalent to just using llvm::support::native.
Simiar possible follow cleanup is that llvm::HashBuilder is now entirely equivalent to just using llvm::HashBuilderImpl directly.

[kazutakahirata]

@zero9178 Thank you so much for reviewing the patch!

Yes, as you've mentioned, I'm planning to:

• remove system_endianness() and
• rename llvm::HashBuilderImpl to llvm::HashBuilder and adjust the comment accordingly.

Aside from these, I'm thinking about:

• renaming llvm::support::endianness to llvm::endianness (as llvm::support::endianness::big is quite mouthful),
• making llvm::endianness an enum class (just like std::endian) as opposed to an enum,
• making all references to big, little, and native fully qualified like llvm::endianness::big to enable mechanical replacements to std::endian::big, etc,
• moving the endianess detection logic in SwapByteOrder.h and the definition of llvm::endianness in Endian.h to llvm/include/llvm/ADT/bit.h.
• defining sys::IsBigEndianHost and sys::IsLittleEndianHost in terms of llvm::endianess, not vice versa, and
• replacing simple if statements like if (sys::IsBigEndianHost) with if (llvm::endianness::native == llvm::endianness::big) to be friendly to readers familiar with C++20.

[dwblaikie]

Sounds like a good direction

[kuhar]

Would be nice to make it enum class at some point

[kazutakahirata]

Yes, indeed. I'm planning to do that. Thank you for the review!