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

[Support] Make readNext default to unaligned #88808

Merged

Conversation

kazutakahirata
Copy link
Contributor

Without this patch, you would typically use readNext as:

readNext<uint32_t, llvm::endianness::little, unaligned>(Ptr)

which is quite mouthful. Since most serialization/deserialization
operations are unaligned accesses, this patch makes the alignment
template parameter default to unaligned, allowing us to say:

readNext<uint32_t, llvm::endianness::little>(Ptr)

I'm including a few examples of migration in this patch. I'll do the
rest in a separate patch.

Note that writeNext already has the same trick for the alignment
template parameter.

Without this patch, you would typically use readNext as:

  readNext<uint32_t, llvm::endianness::little, unaligned>(Ptr)

which is quite mouthful.  Since most serialization/deserialization
operations are unaligned accesses, this patch makes the alignment
template parameter default to unaligned, allowing us to say:

  readNext<uint32_t, llvm::endianness::little>(Ptr)

I'm including a few examples of migration in this patch.  I'll do the
rest in a separate patch.

Note that writeNext already has the same trick for the alignment
template parameter.
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 15, 2024

@llvm/pr-subscribers-llvm-support

Author: Kazu Hirata (kazutakahirata)

Changes

Without this patch, you would typically use readNext as:

readNext<uint32_t, llvm::endianness::little, unaligned>(Ptr)

which is quite mouthful. Since most serialization/deserialization
operations are unaligned accesses, this patch makes the alignment
template parameter default to unaligned, allowing us to say:

readNext<uint32_t, llvm::endianness::little>(Ptr)

I'm including a few examples of migration in this patch. I'll do the
rest in a separate patch.

Note that writeNext already has the same trick for the alignment
template parameter.


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

2 Files Affected:

  • (modified) llvm/include/llvm/Support/Endian.h (+2-2)
  • (modified) llvm/include/llvm/Support/OnDiskHashTable.h (+3-6)
diff --git a/llvm/include/llvm/Support/Endian.h b/llvm/include/llvm/Support/Endian.h
index 1cdb5ca0d5eaa1..30e0852b972c5a 100644
--- a/llvm/include/llvm/Support/Endian.h
+++ b/llvm/include/llvm/Support/Endian.h
@@ -80,8 +80,8 @@ template <typename value_type, std::size_t alignment, typename CharT>
   return ret;
 }
 
-template <typename value_type, endianness endian, std::size_t alignment,
-          typename CharT>
+template <typename value_type, endianness endian,
+          std::size_t alignment = unaligned, typename CharT>
 [[nodiscard]] inline value_type readNext(const CharT *&memory) {
   return readNext<value_type, alignment, CharT>(memory, endian);
 }
diff --git a/llvm/include/llvm/Support/OnDiskHashTable.h b/llvm/include/llvm/Support/OnDiskHashTable.h
index 0a8cbbd8b18832..f6b4055e74de7e 100644
--- a/llvm/include/llvm/Support/OnDiskHashTable.h
+++ b/llvm/include/llvm/Support/OnDiskHashTable.h
@@ -368,14 +368,12 @@ template <typename Info> class OnDiskChainedHashTable {
 
     // 'Items' starts with a 16-bit unsigned integer representing the
     // number of items in this bucket.
-    unsigned Len =
-        endian::readNext<uint16_t, llvm::endianness::little, unaligned>(Items);
+    unsigned Len = endian::readNext<uint16_t, llvm::endianness::little>(Items);
 
     for (unsigned i = 0; i < Len; ++i) {
       // Read the hash.
       hash_value_type ItemHash =
-          endian::readNext<hash_value_type, llvm::endianness::little,
-                           unaligned>(Items);
+          endian::readNext<hash_value_type, llvm::endianness::little>(Items);
 
       // Determine the length of the key and the data.
       const std::pair<offset_type, offset_type> &L =
@@ -473,8 +471,7 @@ class OnDiskIterableChainedHashTable : public OnDiskChainedHashTable<Info> {
         // 'Items' starts with a 16-bit unsigned integer representing the
         // number of items in this bucket.
         NumItemsInBucketLeft =
-            endian::readNext<uint16_t, llvm::endianness::little, unaligned>(
-                Ptr);
+            endian::readNext<uint16_t, llvm::endianness::little>(Ptr);
       }
       Ptr += sizeof(hash_value_type); // Skip the hash.
       // Determine the length of the key and the data.

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

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

Thanks! I just wanted to do the same as I am dabbling with ld.lld --debug-names

@MaskRay
Copy link
Member

MaskRay commented Apr 15, 2024

This is like how to we define packed in llvm/include/llvm/Object/ELFTypes.h. We just assume everything can be unaligned.

There should be no codegen difference to aarch64/x86/ppc64, but certain 32-bit architectures and riscv64 -mstrict-align will have some penalty.

@kazutakahirata kazutakahirata merged commit 568368a into llvm:main Apr 16, 2024
5 of 6 checks passed
@kazutakahirata kazutakahirata deleted the pr_cleanup_readNext_unaligned branch April 16, 2024 02:05
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

3 participants