[TableGen] Simplify generated code for isSubclass #117351
Merged
Conversation
This file contains hidden or 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
Implement isSubclass with some tables and a single call to lower_bound instead of nested switches. Part of the motivation for this is improving compile time when clang-18 is used as a host compiler, since it seems to have trouble with very large switch statements.
|
@llvm/pr-subscribers-tablegen Author: Jay Foad (jayfoad) ChangesImplement isSubclass with some tables and a single call to lower_bound Part of the motivation for this is improving compile time when clang-18 Full diff: https://github.com/llvm/llvm-project/pull/117351.diff 1 Files Affected:
diff --git a/llvm/utils/TableGen/AsmMatcherEmitter.cpp b/llvm/utils/TableGen/AsmMatcherEmitter.cpp
index ade393c11b7a24..68e9b434312a80 100644
--- a/llvm/utils/TableGen/AsmMatcherEmitter.cpp
+++ b/llvm/utils/TableGen/AsmMatcherEmitter.cpp
@@ -2541,7 +2541,6 @@ static void emitIsSubclass(CodeGenTarget &Target,
OS << " if (A == B)\n";
OS << " return true;\n\n";
- bool EmittedSwitch = false;
for (const auto &A : Infos) {
std::vector<StringRef> SuperClasses;
if (A.IsOptional)
@@ -2551,42 +2550,29 @@ static void emitIsSubclass(CodeGenTarget &Target,
SuperClasses.push_back(B.Name);
}
- if (SuperClasses.empty())
+ if (SuperClasses.empty()) {
+ OS << " static constexpr ArrayRef<uint16_t> " << A.Name << "_SuperClasses;\n";
continue;
-
- // If this is the first SuperClass, emit the switch header.
- if (!EmittedSwitch) {
- OS << " switch (A) {\n";
- OS << " default:\n";
- OS << " return false;\n";
- EmittedSwitch = true;
}
- OS << "\n case " << A.Name << ":\n";
-
- if (SuperClasses.size() == 1) {
- OS << " return B == " << SuperClasses.back() << ";\n";
- continue;
- }
-
- if (!SuperClasses.empty()) {
- OS << " switch (B) {\n";
- OS << " default: return false;\n";
- for (StringRef SC : SuperClasses)
- OS << " case " << SC << ": return true;\n";
- OS << " }\n";
- } else {
- // No case statement to emit
- OS << " return false;\n";
- }
+ OS << " static constexpr uint16_t " << A.Name << "_SuperClasses[] = {";
+ ListSeparator LS;
+ for (auto &SC : SuperClasses)
+ OS << LS << SC;
+ OS << "};\n";
}
+ OS << "\n";
- // If there were case statements emitted into the string stream write the
- // default.
- if (EmittedSwitch)
- OS << " }\n";
- else
- OS << " return false;\n";
+ OS << " static constexpr ArrayRef<uint16_t> SuperClassTable[] = {\n";
+ OS << " {}, // InvalidMatchClass\n";
+ OS << " {}, // OptionalMatchClass\n";
+ for (const auto &A : Infos)
+ OS << " " << A.Name << "_SuperClasses,\n";
+ OS << " };\n\n";
+
+ OS << " ArrayRef<uint16_t> SuperClasses = SuperClassTable[(unsigned)A];\n";
+ OS << " const uint16_t *It = lower_bound(SuperClasses, (unsigned)B);\n";
+ OS << " return It != SuperClasses.end() && *It == (unsigned)B;\n";
OS << "}\n\n";
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
IIUC we could have two nested switches (constant time lookup), but now we are using binary search (logarithmic time lookup). Will this cause any time related regressions? |
Good question. I tried measuring this with |
I think I'm seeing around 1% slowdown in some cases, which is not great. Part of the difference is that isSubclass is now simple enough to be inlined, which might affect performance regardless of what algorithm is used inside isSubclass. Here's an example I used for testing: perf stat -e cycles -r 300 llvm-mc -triple x86_64-unknown-unknown test/MC/X86/I86-64.s -filetype=null |
|
The new version uses direct lookup instead of binary search. It seems to be as fast as the nested switch version, maybe even 1% or so faster, but it is hard to tell for sure even when using The largest tables are for AArch64 and AMDGPU, both around 16 to 20 KB, but the nested switches compiled into around 30 to 40 KB of binary code so this is still a win. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/52/builds/4084 Here is the relevant piece of the build log for the reference |
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.
Implement isSubclass with direct lookup into some tables instead of
nested switches.
Part of the motivation for this is improving compile time when clang-18
is used as a host compiler, since it seems to have trouble with very
large switch statements.