Clean up / speed up ULEB128 decoding #73585
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lld-elf @llvm/pr-subscribers-llvm-binary-utilities Author: Adrian Prantl (adrian-prantl) ChangesThis series of patches simplifies the two [U]LEB128 decoder functions in LLVM and makes them ever so slightly faster in the process. As a quick performance test decoding DWARF I instructed dwarfdump to print all DIEs with the name "end" in clang.dSYM without using the accelerator tables: Full diff: https://github.com/llvm/llvm-project/pull/73585.diff 2 Files Affected:
diff --git a/llvm/include/llvm/Support/LEB128.h b/llvm/include/llvm/Support/LEB128.h
index a5d367279aefe64..3d5e98c4b2cddee 100644
--- a/llvm/include/llvm/Support/LEB128.h
+++ b/llvm/include/llvm/Support/LEB128.h
@@ -125,29 +125,30 @@ inline unsigned encodeULEB128(uint64_t Value, uint8_t *p,
}
/// Utility function to decode a ULEB128 value.
+///
+/// If \p error is non-null, it will point to a static error message,
+/// if an error occured. It will not be modified on success.
inline uint64_t decodeULEB128(const uint8_t *p, unsigned *n = nullptr,
const uint8_t *end = nullptr,
const char **error = nullptr) {
const uint8_t *orig_p = p;
uint64_t Value = 0;
unsigned Shift = 0;
- if (error)
- *error = nullptr;
do {
- if (p == end) {
+ if (LLVM_UNLIKELY(p == end)) {
if (error)
*error = "malformed uleb128, extends past end";
- if (n)
- *n = (unsigned)(p - orig_p);
- return 0;
+ Value = 0;
+ break;
}
uint64_t Slice = *p & 0x7f;
- if ((Shift >= 64 && Slice != 0) || Slice << Shift >> Shift != Slice) {
+ if (LLVM_UNLIKELY(Shift >= 63) &&
+ ((Shift == 63 && ((Slice << Shift) >> Shift) != Slice) ||
+ (Shift > 63 && Slice != 0))) {
if (error)
*error = "uleb128 too big for uint64";
- if (n)
- *n = (unsigned)(p - orig_p);
- return 0;
+ Value = 0;
+ break;
}
Value += Slice << Shift;
Shift += 7;
@@ -158,6 +159,9 @@ inline uint64_t decodeULEB128(const uint8_t *p, unsigned *n = nullptr,
}
/// Utility function to decode a SLEB128 value.
+///
+/// If \p error is non-null, it will point to a static error message,
+/// if an error occured. It will not be modified on success.
inline int64_t decodeSLEB128(const uint8_t *p, unsigned *n = nullptr,
const uint8_t *end = nullptr,
const char **error = nullptr) {
@@ -165,10 +169,8 @@ inline int64_t decodeSLEB128(const uint8_t *p, unsigned *n = nullptr,
int64_t Value = 0;
unsigned Shift = 0;
uint8_t Byte;
- if (error)
- *error = nullptr;
do {
- if (p == end) {
+ if (LLVM_UNLIKELY(p == end)) {
if (error)
*error = "malformed sleb128, extends past end";
if (n)
@@ -177,8 +179,9 @@ inline int64_t decodeSLEB128(const uint8_t *p, unsigned *n = nullptr,
}
Byte = *p;
uint64_t Slice = Byte & 0x7f;
- if ((Shift >= 64 && Slice != (Value < 0 ? 0x7f : 0x00)) ||
- (Shift == 63 && Slice != 0 && Slice != 0x7f)) {
+ if (LLVM_UNLIKELY(Shift >= 63) &&
+ ((Shift == 63 && Slice != 0 && Slice != 0x7f) ||
+ (Shift > 63 && Slice != (Value < 0 ? 0x7f : 0x00)))) {
if (error)
*error = "sleb128 too big for int64";
if (n)
diff --git a/llvm/lib/Object/MachOObjectFile.cpp b/llvm/lib/Object/MachOObjectFile.cpp
index aa57de16ed18f44..11ad8aeae65da5d 100644
--- a/llvm/lib/Object/MachOObjectFile.cpp
+++ b/llvm/lib/Object/MachOObjectFile.cpp
@@ -2996,7 +2996,7 @@ void ExportEntry::pushNode(uint64_t offset) {
ErrorAsOutParameter ErrAsOutParam(E);
const uint8_t *Ptr = Trie.begin() + offset;
NodeState State(Ptr);
- const char *error;
+ const char *error = nullptr;
uint64_t ExportInfoSize = readULEB128(State.Current, &error);
if (error) {
*E = malformedError("export info size " + Twine(error) +
|
|
@llvm/pr-subscribers-llvm-support Author: Adrian Prantl (adrian-prantl) ChangesThis series of patches simplifies the two [U]LEB128 decoder functions in LLVM and makes them ever so slightly faster in the process. As a quick performance test decoding DWARF I instructed dwarfdump to print all DIEs with the name "end" in clang.dSYM without using the accelerator tables: Full diff: https://github.com/llvm/llvm-project/pull/73585.diff 2 Files Affected:
diff --git a/llvm/include/llvm/Support/LEB128.h b/llvm/include/llvm/Support/LEB128.h
index a5d367279aefe64..3d5e98c4b2cddee 100644
--- a/llvm/include/llvm/Support/LEB128.h
+++ b/llvm/include/llvm/Support/LEB128.h
@@ -125,29 +125,30 @@ inline unsigned encodeULEB128(uint64_t Value, uint8_t *p,
}
/// Utility function to decode a ULEB128 value.
+///
+/// If \p error is non-null, it will point to a static error message,
+/// if an error occured. It will not be modified on success.
inline uint64_t decodeULEB128(const uint8_t *p, unsigned *n = nullptr,
const uint8_t *end = nullptr,
const char **error = nullptr) {
const uint8_t *orig_p = p;
uint64_t Value = 0;
unsigned Shift = 0;
- if (error)
- *error = nullptr;
do {
- if (p == end) {
+ if (LLVM_UNLIKELY(p == end)) {
if (error)
*error = "malformed uleb128, extends past end";
- if (n)
- *n = (unsigned)(p - orig_p);
- return 0;
+ Value = 0;
+ break;
}
uint64_t Slice = *p & 0x7f;
- if ((Shift >= 64 && Slice != 0) || Slice << Shift >> Shift != Slice) {
+ if (LLVM_UNLIKELY(Shift >= 63) &&
+ ((Shift == 63 && ((Slice << Shift) >> Shift) != Slice) ||
+ (Shift > 63 && Slice != 0))) {
if (error)
*error = "uleb128 too big for uint64";
- if (n)
- *n = (unsigned)(p - orig_p);
- return 0;
+ Value = 0;
+ break;
}
Value += Slice << Shift;
Shift += 7;
@@ -158,6 +159,9 @@ inline uint64_t decodeULEB128(const uint8_t *p, unsigned *n = nullptr,
}
/// Utility function to decode a SLEB128 value.
+///
+/// If \p error is non-null, it will point to a static error message,
+/// if an error occured. It will not be modified on success.
inline int64_t decodeSLEB128(const uint8_t *p, unsigned *n = nullptr,
const uint8_t *end = nullptr,
const char **error = nullptr) {
@@ -165,10 +169,8 @@ inline int64_t decodeSLEB128(const uint8_t *p, unsigned *n = nullptr,
int64_t Value = 0;
unsigned Shift = 0;
uint8_t Byte;
- if (error)
- *error = nullptr;
do {
- if (p == end) {
+ if (LLVM_UNLIKELY(p == end)) {
if (error)
*error = "malformed sleb128, extends past end";
if (n)
@@ -177,8 +179,9 @@ inline int64_t decodeSLEB128(const uint8_t *p, unsigned *n = nullptr,
}
Byte = *p;
uint64_t Slice = Byte & 0x7f;
- if ((Shift >= 64 && Slice != (Value < 0 ? 0x7f : 0x00)) ||
- (Shift == 63 && Slice != 0 && Slice != 0x7f)) {
+ if (LLVM_UNLIKELY(Shift >= 63) &&
+ ((Shift == 63 && Slice != 0 && Slice != 0x7f) ||
+ (Shift > 63 && Slice != (Value < 0 ? 0x7f : 0x00)))) {
if (error)
*error = "sleb128 too big for int64";
if (n)
diff --git a/llvm/lib/Object/MachOObjectFile.cpp b/llvm/lib/Object/MachOObjectFile.cpp
index aa57de16ed18f44..11ad8aeae65da5d 100644
--- a/llvm/lib/Object/MachOObjectFile.cpp
+++ b/llvm/lib/Object/MachOObjectFile.cpp
@@ -2996,7 +2996,7 @@ void ExportEntry::pushNode(uint64_t offset) {
ErrorAsOutParameter ErrAsOutParam(E);
const uint8_t *Ptr = Trie.begin() + offset;
NodeState State(Ptr);
- const char *error;
+ const char *error = nullptr;
uint64_t ExportInfoSize = readULEB128(State.Current, &error);
if (error) {
*E = malformedError("export info size " + Twine(error) +
|
MaskRay
approved these changes
Nov 28, 2023
llvm/include/llvm/Support/LEB128.h
Outdated
| } | ||
| uint64_t Slice = *p & 0x7f; | ||
| if ((Shift >= 64 && Slice != 0) || Slice << Shift >> Shift != Slice) { | ||
| if (LLVM_UNLIKELY(Shift >= 63) && | ||
| ((Shift == 63 && ((Slice << Shift) >> Shift) != Slice) || |
There was a problem hiding this comment.
The inner paren in (Slice << Shift) >> Shift can be omitted.
There was a problem hiding this comment.
I only added this for better readability. At first glance x << Shift >> Shift looks like a noop otherwise.
There was a problem hiding this comment.
Thinking about it though, the parenthesis don't make this any clearer.
adrian-prantl
force-pushed
the
uleb-decoding
branch
from
4374a28
to
8721313
Compare
JDevlieghere
approved these changes
Nov 28, 2023
This change removes an unnecessary branch from a hot path. It's also questionable API to override any previous error unconditonally.
Previously the overflow check was done for every byte even though it is only needed for the case where Shift == 63.
felipepiovezan
approved these changes
Nov 29, 2023
adrian-prantl
force-pushed
the
uleb-decoding
branch
from
8721313
to
a9f9736
Compare
|
I manually pushed this in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series of patches simplifies the two [U]LEB128 decoder functions in LLVM and makes them ever so slightly faster in the process.
As a quick performance test decoding DWARF I instructed dwarfdump to print all DIEs with the name "end" in clang.dSYM without using the accelerator tables: