Patch series to reapply #118734 and substantially improve it #120534
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-xtensa @llvm/pr-subscribers-backend-arm Author: Chandler Carruth (chandlerc) ChangesThis PR is for a patch series of 5 patches to try and fully address the Clang builtin string tables. I'm posting it as a single PR to give context to the series and explain what the overarching goal is, but each of the 5 patches is designed to be reasonable to review on its own and land on its own. The only issue I expect is that MSVC 16.9 on one build bot is likely to miscompile the intermediate steps, and MSVC will consume a large amount of memory during compilation until all the patches are in place. That said, I'm happy to have each patch reviewed here, or I can post independent PRs for each patch, whichever would work best for reviewers. The overarching goal of this patch series is to re-land the port of Clang's builtin tables to use offsets into string tables rather than containing dynamically relocated pointers to individual strings. A naive port as in #119638 has some serious issues due to forming extraordinarily large string tables, and many of them. To address this, subsequent patches restructure the builtin tables to make them smaller through sharding, use of TableGen-ed string tables without duplicates, and extracting a common prefix that can be dynamically added. The first patch results in a string table over 500KiB, but at the end the largest is well below 200KiB. Combined, this patch series reduces the readonly data used by Clang's builtins by roughly 50% in addition to removing the dynamic relocations needed. The net result is a 30% reduction in the binary's dynamic relocation metadata, and a 2% reduction in the total binary size. An overview of the various patches in the series:
Each patch has a somewhat more detailed description in its commit message. The struture at the end opens up a lot of room to make more improvements, especially leveraging TableGen more heavily by having the ability to fall back to X-macro structures where necessary. I have a partial patch to move the main builtins to TableGen as well, but that will be more involved and unlikely to have as significant of impact on string literal size or binary size. Patch is 115.77 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/120534.diff 62 Files Affected:
diff --git a/clang/include/clang/Basic/Builtins.h b/clang/include/clang/Basic/Builtins.h
index e27d8ccce73664..28daa4e1aafe48 100644
--- a/clang/include/clang/Basic/Builtins.h
+++ b/clang/include/clang/Basic/Builtins.h
@@ -18,6 +18,7 @@
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringTable.h"
#include <cstring>
// VC++ defines 'alloca' as an object-like macro, which interferes with our
@@ -55,6 +56,7 @@ struct HeaderDesc {
#undef HEADER
} ID;
+ constexpr HeaderDesc() : ID() {}
constexpr HeaderDesc(HeaderID ID) : ID(ID) {}
const char *getName() const;
@@ -68,14 +70,151 @@ enum ID {
FirstTSBuiltin
};
+struct InfosShard;
+
+/// The info used to represent each builtin.
struct Info {
- llvm::StringLiteral Name;
- const char *Type, *Attributes;
- const char *Features;
+ // Rather than store pointers to the string literals describing these four
+ // aspects of builtins, we store offsets into a common string table.
+ struct StrOffsets {
+ llvm::StringTable::Offset Name;
+ llvm::StringTable::Offset Type;
+ llvm::StringTable::Offset Attributes;
+ llvm::StringTable::Offset Features;
+ } Offsets;
+
HeaderDesc Header;
LanguageID Langs;
+
+ /// Get the name for the builtin represented by this `Info` object.
+ ///
+ /// Must be provided the `Shard` for this `Info` object.
+ std::string getName(const InfosShard &Shard) const;
};
+/// A constexpr function to construct an infos array from X-macros.
+///
+/// The input array uses the same data structure, but the offsets are actually
+/// _lengths_ when input. This is all we can compute from the X-macro approach
+/// to builtins. This function will convert these lengths into actual offsets to
+/// a string table built up through sequentially appending strings with the
+/// given lengths.
+template <size_t N>
+static constexpr std::array<Info, N> MakeInfos(std::array<Info, N> Infos) {
+ // Translate lengths to offsets. We start past the initial empty string at
+ // offset zero.
+ unsigned Offset = 1;
+ for (auto &I : Infos) {
+ Info::StrOffsets NewOffsets = {};
+ NewOffsets.Name = Offset;
+ Offset += I.Offsets.Name.value();
+ NewOffsets.Type = Offset;
+ Offset += I.Offsets.Type.value();
+ NewOffsets.Attributes = Offset;
+ Offset += I.Offsets.Attributes.value();
+ NewOffsets.Features = Offset;
+ Offset += I.Offsets.Features.value();
+ I.Offsets = NewOffsets;
+ }
+ return Infos;
+}
+
+/// A shard of a target's builtins string table and info.
+///
+/// Target builtins are sharded across multiple tables due to different
+/// structures, origins, and also to improve the overall scaling by avoiding a
+/// single table across all builtins.
+struct InfosShard {
+ const llvm::StringTable *Strings;
+ llvm::ArrayRef<Info> Infos;
+
+ llvm::StringLiteral NamePrefix = "";
+};
+
+// A detail macro used below to emit a string literal that, after string literal
+// concatenation, ends up triggering the `-Woverlength-strings` warning. While
+// the warning is useful in general to catch accidentally excessive strings,
+// here we are creating them intentionally.
+//
+// This relies on a subtle aspect of `_Pragma`: that the *diagnostic* ones don't
+// turn into actual tokens that would disrupt string literal concatenation.
+#ifdef __clang__
+#define CLANG_BUILTIN_DETAIL_STR_TABLE(S) \
+ _Pragma("clang diagnostic push") \
+ _Pragma("clang diagnostic ignored \"-Woverlength-strings\"") \
+ S _Pragma("clang diagnostic pop")
+#else
+#define CLANG_BUILTIN_DETAIL_STR_TABLE(S) S
+#endif
+
+// We require string tables to start with an empty string so that a `0` offset
+// can always be used to refer to an empty string. To satisfy that when building
+// string tables with X-macros, we use this start macro prior to expanding the
+// X-macros.
+#define CLANG_BUILTIN_STR_TABLE_START CLANG_BUILTIN_DETAIL_STR_TABLE("\0")
+
+// A macro that can be used with `Builtins.def` and similar files as an X-macro
+// to add the string arguments to a builtin string table. This is typically the
+// target for the `BUILTIN`, `LANGBUILTIN`, or `LIBBUILTIN` macros in those
+// files.
+#define CLANG_BUILTIN_STR_TABLE(ID, TYPE, ATTRS) \
+ CLANG_BUILTIN_DETAIL_STR_TABLE(#ID "\0" TYPE "\0" ATTRS "\0" /*FEATURE*/ "\0")
+
+// A macro that can be used with target builtin `.def` and `.inc` files as an
+// X-macro to add the string arguments to a builtin string table. this is
+// typically the target for the `TARGET_BUILTIN` macro.
+#define CLANG_TARGET_BUILTIN_STR_TABLE(ID, TYPE, ATTRS, FEATURE) \
+ CLANG_BUILTIN_DETAIL_STR_TABLE(#ID "\0" TYPE "\0" ATTRS "\0" FEATURE "\0")
+
+// A macro that can be used with target builtin `.def` and `.inc` files as an
+// X-macro to add the string arguments to a builtin string table. this is
+// typically the target for the `TARGET_HEADER_BUILTIN` macro. We can't delegate
+// to `TARGET_BUILTIN` because the `FEATURE` string changes position.
+#define CLANG_TARGET_HEADER_BUILTIN_STR_TABLE(ID, TYPE, ATTRS, HEADER, LANGS, \
+ FEATURE) \
+ CLANG_BUILTIN_DETAIL_STR_TABLE(#ID "\0" TYPE "\0" ATTRS "\0" FEATURE "\0")
+
+// A detail macro used internally to compute the desired string table
+// `StrOffsets` struct for arguments to `MakeInfos`.
+#define CLANG_BUILTIN_DETAIL_STR_OFFSETS(ID, TYPE, ATTRS) \
+ Builtin::Info::StrOffsets { \
+ sizeof(#ID), sizeof(TYPE), sizeof(ATTRS), sizeof("") \
+ }
+
+// A detail macro used internally to compute the desired string table
+// `StrOffsets` struct for arguments to `Storage::Make`.
+#define CLANG_TARGET_BUILTIN_DETAIL_STR_OFFSETS(ID, TYPE, ATTRS, FEATURE) \
+ Builtin::Info::StrOffsets { \
+ sizeof(#ID), sizeof(TYPE), sizeof(ATTRS), sizeof(FEATURE) \
+ }
+
+// A set of macros that can be used with builtin `.def' files as an X-macro to
+// create an `Info` struct for a particular builtin. It both computes the
+// `StrOffsets` value for the string table (the lengths here, translated to
+// offsets by the `MakeInfos` function), and the other metadata for each
+// builtin.
+//
+// There is a corresponding macro for each of `BUILTIN`, `LANGBUILTIN`,
+// `LIBBUILTIN`, `TARGET_BUILTIN`, and `TARGET_HEADER_BUILTIN`.
+#define CLANG_BUILTIN_ENTRY(ID, TYPE, ATTRS) \
+ Builtin::Info{CLANG_BUILTIN_DETAIL_STR_OFFSETS(ID, TYPE, ATTRS), \
+ HeaderDesc::NO_HEADER, ALL_LANGUAGES},
+#define CLANG_LANGBUILTIN_ENTRY(ID, TYPE, ATTRS, LANG) \
+ Builtin::Info{CLANG_BUILTIN_DETAIL_STR_OFFSETS(ID, TYPE, ATTRS), \
+ HeaderDesc::NO_HEADER, LANG},
+#define CLANG_LIBBUILTIN_ENTRY(ID, TYPE, ATTRS, HEADER, LANG) \
+ Builtin::Info{CLANG_BUILTIN_DETAIL_STR_OFFSETS(ID, TYPE, ATTRS), \
+ HeaderDesc::HEADER, LANG},
+#define CLANG_TARGET_BUILTIN_ENTRY(ID, TYPE, ATTRS, FEATURE) \
+ Builtin::Info{ \
+ CLANG_TARGET_BUILTIN_DETAIL_STR_OFFSETS(ID, TYPE, ATTRS, FEATURE), \
+ HeaderDesc::NO_HEADER, ALL_LANGUAGES},
+#define CLANG_TARGET_HEADER_BUILTIN_ENTRY(ID, TYPE, ATTRS, HEADER, LANG, \
+ FEATURE) \
+ Builtin::Info{ \
+ CLANG_TARGET_BUILTIN_DETAIL_STR_OFFSETS(ID, TYPE, ATTRS, FEATURE), \
+ HeaderDesc::HEADER, LANG},
+
/// Holds information about both target-independent and
/// target-specific builtins, allowing easy queries by clients.
///
@@ -83,11 +222,16 @@ struct Info {
/// AuxTSRecords. Their IDs are shifted up by TSRecords.size() and need to
/// be translated back with getAuxBuiltinID() before use.
class Context {
- llvm::ArrayRef<Info> TSRecords;
- llvm::ArrayRef<Info> AuxTSRecords;
+ llvm::SmallVector<InfosShard> BuiltinShards;
+
+ llvm::SmallVector<InfosShard> TargetShards;
+ llvm::SmallVector<InfosShard> AuxTargetShards;
+
+ unsigned NumTargetBuiltins = 0;
+ unsigned NumAuxTargetBuiltins = 0;
public:
- Context() = default;
+ Context();
/// Perform target-specific initialization
/// \param AuxTarget Target info to incorporate builtins from. May be nullptr.
@@ -100,10 +244,17 @@ class Context {
/// Return the identifier name for the specified builtin,
/// e.g. "__builtin_abs".
- llvm::StringRef getName(unsigned ID) const { return getRecord(ID).Name; }
+ std::string getName(unsigned ID) const;
+
+ /// Return the identifier name for the specified builtin inside single quotes
+ /// for a diagnostic, e.g. "'__builtin_abs'".
+ std::string getQuotedName(unsigned ID) const;
/// Get the type descriptor string for the specified builtin.
- const char *getTypeString(unsigned ID) const { return getRecord(ID).Type; }
+ const char *getTypeString(unsigned ID) const;
+
+ /// Get the attributes descriptor string for the specified builtin.
+ const char *getAttributesString(unsigned ID) const;
/// Return true if this function is a target-specific builtin.
bool isTSBuiltin(unsigned ID) const {
@@ -112,40 +263,40 @@ class Context {
/// Return true if this function has no side effects.
bool isPure(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'U') != nullptr;
+ return strchr(getAttributesString(ID), 'U') != nullptr;
}
/// Return true if this function has no side effects and doesn't
/// read memory.
bool isConst(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'c') != nullptr;
+ return strchr(getAttributesString(ID), 'c') != nullptr;
}
/// Return true if we know this builtin never throws an exception.
bool isNoThrow(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'n') != nullptr;
+ return strchr(getAttributesString(ID), 'n') != nullptr;
}
/// Return true if we know this builtin never returns.
bool isNoReturn(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'r') != nullptr;
+ return strchr(getAttributesString(ID), 'r') != nullptr;
}
/// Return true if we know this builtin can return twice.
bool isReturnsTwice(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'j') != nullptr;
+ return strchr(getAttributesString(ID), 'j') != nullptr;
}
/// Returns true if this builtin does not perform the side-effects
/// of its arguments.
bool isUnevaluated(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'u') != nullptr;
+ return strchr(getAttributesString(ID), 'u') != nullptr;
}
/// Return true if this is a builtin for a libc/libm function,
/// with a "__builtin_" prefix (e.g. __builtin_abs).
bool isLibFunction(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'F') != nullptr;
+ return strchr(getAttributesString(ID), 'F') != nullptr;
}
/// Determines whether this builtin is a predefined libc/libm
@@ -156,21 +307,21 @@ class Context {
/// they do not, but they are recognized as builtins once we see
/// a declaration.
bool isPredefinedLibFunction(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'f') != nullptr;
+ return strchr(getAttributesString(ID), 'f') != nullptr;
}
/// Returns true if this builtin requires appropriate header in other
/// compilers. In Clang it will work even without including it, but we can emit
/// a warning about missing header.
bool isHeaderDependentFunction(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'h') != nullptr;
+ return strchr(getAttributesString(ID), 'h') != nullptr;
}
/// Determines whether this builtin is a predefined compiler-rt/libgcc
/// function, such as "__clear_cache", where we know the signature a
/// priori.
bool isPredefinedRuntimeFunction(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'i') != nullptr;
+ return strchr(getAttributesString(ID), 'i') != nullptr;
}
/// Determines whether this builtin is a C++ standard library function
@@ -178,7 +329,7 @@ class Context {
/// specialization, where the signature is determined by the standard library
/// declaration.
bool isInStdNamespace(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'z') != nullptr;
+ return strchr(getAttributesString(ID), 'z') != nullptr;
}
/// Determines whether this builtin can have its address taken with no
@@ -192,33 +343,33 @@ class Context {
/// Determines whether this builtin has custom typechecking.
bool hasCustomTypechecking(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 't') != nullptr;
+ return strchr(getAttributesString(ID), 't') != nullptr;
}
/// Determines whether a declaration of this builtin should be recognized
/// even if the type doesn't match the specified signature.
bool allowTypeMismatch(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'T') != nullptr ||
+ return strchr(getAttributesString(ID), 'T') != nullptr ||
hasCustomTypechecking(ID);
}
/// Determines whether this builtin has a result or any arguments which
/// are pointer types.
bool hasPtrArgsOrResult(unsigned ID) const {
- return strchr(getRecord(ID).Type, '*') != nullptr;
+ return strchr(getTypeString(ID), '*') != nullptr;
}
/// Return true if this builtin has a result or any arguments which are
/// reference types.
bool hasReferenceArgsOrResult(unsigned ID) const {
- return strchr(getRecord(ID).Type, '&') != nullptr ||
- strchr(getRecord(ID).Type, 'A') != nullptr;
+ return strchr(getTypeString(ID), '&') != nullptr ||
+ strchr(getTypeString(ID), 'A') != nullptr;
}
/// If this is a library function that comes from a specific
/// header, retrieve that header name.
const char *getHeaderName(unsigned ID) const {
- return getRecord(ID).Header.getName();
+ return getInfo(ID).Header.getName();
}
/// Determine whether this builtin is like printf in its
@@ -243,27 +394,25 @@ class Context {
/// Such functions can be const when the MathErrno lang option and FP
/// exceptions are disabled.
bool isConstWithoutErrnoAndExceptions(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'e') != nullptr;
+ return strchr(getAttributesString(ID), 'e') != nullptr;
}
bool isConstWithoutExceptions(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'g') != nullptr;
+ return strchr(getAttributesString(ID), 'g') != nullptr;
}
- const char *getRequiredFeatures(unsigned ID) const {
- return getRecord(ID).Features;
- }
+ const char *getRequiredFeatures(unsigned ID) const;
unsigned getRequiredVectorWidth(unsigned ID) const;
/// Return true if builtin ID belongs to AuxTarget.
bool isAuxBuiltinID(unsigned ID) const {
- return ID >= (Builtin::FirstTSBuiltin + TSRecords.size());
+ return ID >= (Builtin::FirstTSBuiltin + NumTargetBuiltins);
}
/// Return real builtin ID (i.e. ID it would have during compilation
/// for AuxTarget).
- unsigned getAuxBuiltinID(unsigned ID) const { return ID - TSRecords.size(); }
+ unsigned getAuxBuiltinID(unsigned ID) const { return ID - NumTargetBuiltins; }
/// Returns true if this is a libc/libm function without the '__builtin_'
/// prefix.
@@ -275,16 +424,19 @@ class Context {
/// Return true if this function can be constant evaluated by Clang frontend.
bool isConstantEvaluated(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'E') != nullptr;
+ return strchr(getAttributesString(ID), 'E') != nullptr;
}
/// Returns true if this is an immediate (consteval) function
bool isImmediate(unsigned ID) const {
- return strchr(getRecord(ID).Attributes, 'G') != nullptr;
+ return strchr(getAttributesString(ID), 'G') != nullptr;
}
private:
- const Info &getRecord(unsigned ID) const;
+ std::pair<const InfosShard &, const Info &>
+ getShardAndInfo(unsigned ID) const;
+
+ const Info &getInfo(unsigned ID) const { return getShardAndInfo(ID).second; }
/// Helper function for isPrintfLike and isScanfLike.
bool isLike(unsigned ID, unsigned &FormatIdx, bool &HasVAListArg,
diff --git a/clang/include/clang/Basic/BuiltinsNEON.def b/clang/include/clang/Basic/BuiltinsNEON.def
deleted file mode 100644
index 9627005ba9824e..00000000000000
--- a/clang/include/clang/Basic/BuiltinsNEON.def
+++ /dev/null
@@ -1,22 +0,0 @@
-//===--- BuiltinsNEON.def - NEON Builtin function database ------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// This file defines the NEON-specific builtin function database. Users of
-// this file must define the BUILTIN macro to make use of this information.
-//
-//===----------------------------------------------------------------------===//
-
-// The format of this database matches clang/Basic/Builtins.def.
-
-#define GET_NEON_BUILTINS
-#include "clang/Basic/arm_neon.inc"
-#include "clang/Basic/arm_fp16.inc"
-#undef GET_NEON_BUILTINS
-
-#undef BUILTIN
-#undef TARGET_BUILTIN
diff --git a/clang/include/clang/Basic/BuiltinsPPC.def b/clang/include/clang/Basic/BuiltinsPPC.def
index 161df386f00f03..bb7d54bbb793eb 100644
--- a/clang/include/clang/Basic/BuiltinsPPC.def
+++ b/clang/include/clang/Basic/BuiltinsPPC.def
@@ -1138,5 +1138,6 @@ UNALIASED_CUSTOM_BUILTIN(mma_pmxvbf16ger2nn, "vW512*VVi15i15i3", true,
// FIXME: Obviously incomplete.
#undef BUILTIN
+#undef TARGET_BUILTIN
#undef CUSTOM_BUILTIN
#undef UNALIASED_CUSTOM_BUILTIN
diff --git a/clang/include/clang/Basic/BuiltinsRISCVVector.def b/clang/include/clang/Basic/BuiltinsRISCVVector.def
deleted file mode 100644
index 6dfa87a1a1d313..00000000000000
--- a/clang/include/clang/Basic/BuiltinsRISCVVector.def
+++ /dev/null
@@ -1,22 +0,0 @@
-//==- BuiltinsRISCVVector.def - RISC-V Vector Builtin Database ---*- C++ -*-==//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// This file defines the RISC-V-specific builtin function database. Users of
-// this file must define the BUILTIN macro to make use of this information.
-//
-//===----------------------------------------------------------------------===//
-
-#if defined(BUILTIN) && !defined(TARGET_BUILTIN)
-# define TARGET_BUILTIN(ID, TYPE, ATTRS, FEATURE) BUILTIN(ID, TYPE, ATTRS)
-#endif
-
-#include "clang/Basic/riscv_vector_builtins.inc"
-#include "clang/Basic/riscv_sifive_vector_builtins.inc"
-
-#undef BUILTIN
-#undef TARGET_BUILTIN
diff --git a/clang/include/clang/Basic/TargetBuiltins.h b/clang/include/clang/Basic/TargetBuiltins.h
index a14fd2c4b224d8..238f7510a8ed60 100644
--- a/clang/include/clang/Basic/TargetBuiltins.h
+++ b/clang/include/clang/Basic/TargetBuiltins.h
@@ -26,9 +26,12 @@ namespace clang {
namespace NEON {
enum {
LastTIBuiltin = clang::Builtin::FirstTSBuiltin - 1,
-#define BUILTIN(ID, TYPE, ATTRS) BI##ID,
-#define TARGET_BUILTIN(ID, TYPE, ATTRS, FEATURE) BI##ID,
-#include "clang/Basic/BuiltinsNEON.def"
+#define GET_NEON_BUILTIN_ENUMERATORS
+#include "clang/Basic/arm_neon.inc"
+ FirstFp16Builtin,
+ LastNeonBuiltin = FirstFp16Builtin - 1,
+#include "clang/Basic/arm_fp16.inc"
+#undef GET_NEON_BUILTIN_ENUMERATORS
FirstTSBuiltin
};
}
@@ -47,9 +50,16 @@ namespace clang {
namespace SVE {
enum {
LastNEONBuiltin = NEON::FirstTSBuiltin - 1,
-#define BUILTIN(ID, TYPE, ATTRS) BI##ID,
+#define GET_SVE_BUILTIN_ENUMERATORS
+#include "clang/Basic/arm_sve_builtins.inc"
+#undef GET_...
[truncated]
|
|
@dyung - I believe this PR may be a credible path to address the issues hit with your MSVC builders, would appreciate any help testing it in advance if possible. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
chandlerc
changed the title
Sure, I'll give it a try |
chandlerc
force-pushed
the
prefix-arm-rv
branch
2 times, most recently
from
b71ac53 to
2bcc4e5
Compare
You seem to still be working on it, can you tell me which commit I should try building/testing when you are done? |
Sorry, was just tidying up things that I missed until the PR was uploaded. Only rebasing on top-of-tree and a clang-format issue, nothing that changes functionality. And I think all done now so you should be good-to-go with the top of this PR (I think |
I'll try it and let you know. Give me about an hour or so. |
…d MSVC (#123548) Historically, the main example of *very* large string tables used the `EmitCharArray` to work around MSVC limitations with string literals, but that was switched (without removing the API) in order to consolidate on a nicer emission primitive. While this large string table in `IntrinsicsImpl.inc` seems to compile correctly on MSVC without the work around in `EmitCharArray` (and that this PR adds back to the nicer emission path), other users have repeatedly hit this MSVC limitation as you can see in the discussion on PR llvm/llvm-project#120534. This PR teaches the string offset table emission to look at the size of the table and switch to the char array emission strategy when the table becomes too large. This work around does have the downside of making compile times worse for large string tables, but that appears unavoidable until we can identify known good MSVC versions and switch to requiring them for all LLVM users. It also reduces searchability of the generated string table -- I looked at emitting a comment with each string but it is tricky because the escaping rules for an inline comment are different from those of of a string literal, and there's no real way to turn the string literal into a comment. While improving the output in this way, also clean up the output to not emit an extraneous empty string at the end of the string table, and update the `StringTable` class to not look for that. It isn't actually used by anything and is wasteful. This PR also switches the `IntrinsicsImpl.inc` string tables over to the new `StringTable` runtime abstraction. I didn't want to do this until landing the MSVC workaround in case it caused even this example to start hitting the MSVC bug, but I wanted to switch here so that I could simplify the API for emitting the string table with the workaround present. With the two different emission strategies, its important to use a very exact syntax and that seems better encapsulated in the API. Last but not least, the `SDNodeInfoEmitter` is updated, including its tests to match the new output. This PR should unblock landing llvm/llvm-project#120534 and letting us switch all of Clang's builtins to use string tables. That PR has all the details motivating the overall effort. Follow-up patches will try to consolidate the remaining users onto the single interface, but those at least were easy to separate into follow-ups and keep this PR somewhat smaller.
chandlerc
force-pushed
the
prefix-arm-rv
branch
from
a83165a to
2b85083
Compare
|
All of the dependent changes have landed, and I've re-structured this patch series to hopefully be easier to review. I still don't have a good way to land this incrementally, as the tools to work around MSVC limitations are only available when using TableGen-ed string tables, and those aren't available at first. I could split out some of the later patches, but those are smaller and probably doesn't help as much. But the patches should be meaningful chunks to review here. There have been quite a few changes, so this does probably warrant a re-review from folks. Sorry for that. @dyung -- as always, appreciate help verifying that this isn't going to break MSVC builds for you. The top of this PR should be up to date and ready to test. |
Happy to do so! Some updates on our end, I think that we have finished updating our internal machines with a newer build of Visual Studio 2019, but I need to confirm it. Our upstream bot has not yet been updated though, but I can do that pretty quickly/easily. I will test your changes on both versions tomorrow and let you know the results. |
I ran the most recent changes in this PR (2b85083) and can confirm that all tests passed on both the original and newer version of Visual Studio 2019 that we are upgrading to. |
|
I know it's only been a few days, but pinging in the hope of landing this week... This seems to finally be in a good state and is somewhat hard to keep rebasing. Happy to do anything I can to help make review easier. |
|
Will LLVM 20 miss this? |
|
You'd have to merge and request a backport pretty quickly. Historically features could still be argued to land between rc1 (which is imminent) and rc2, but the windows is definitely closing fast |
|
Given the benefit to binary size and compile time in things like Erich already reviewed an earlier version. Maybe @rnk can help with reviews? |
|
RC1 coming soon |
There was a problem hiding this comment.
Thanks for optimizing the builtins! I feel like builtins have grown significantly since adding RISC-V and ARM MVE intrinsics, and few people until now have stopped to re-evaluate how we represent these things.
While I was watching these clearly generated Intrinsic files land in Clang, I was thinking, these are clearly generated from a data model using a script. Wouldn't it be more efficient to encode the underlying data model for these operations using a more normalized representation? I proposed this at one point, and someone from Intel assured me that these long repetitive instructions and intrinsics are hand-maintained. It must be true at some level. Regardless, this change seems like a positive step towards renormalizing the otherwise denormalized builtins databse.
There's a lot of code and I'm sure there are more nits I could pick out, but I think it makes sense to reland this with the more sharded approach and get some feedback in tree.
| collectBuiltins(BuiltinRecord, Builtins); | ||
| } | ||
|
|
||
| // Now collect (and count) the prefixed builtins. | ||
| unsigned NumPrefixedBuiltins = Builtins.size(); | ||
| const Record *first_prefix = nullptr; |
There was a problem hiding this comment.
Super nit, just to prove I read all the code: Why the change in style here? FirstPrefix?
There was a problem hiding this comment.
Doh, just an accident, sorry. Out of the habits of LLVM style, fixed.
|
Thanks @rnk ! I've fixed the one style nit (doh!) and a few surrounding variables. I'm working on rebasing everything now. But one specific question: would you prefer me to land as a series of commits or a single squashed commit for the entire PR? I'm happy either way. My mild preference is to prefer the series of commits, but open to suggestions here. |
This both reapplies llvm#118734, the initial attempt at this, and updates it significantly. First, it uses the newly added `StringTable` abstraction for string tables, and simplifies the construction to build the string table and info arrays separately. This should reduce any `constexpr` compile time memory or CPU cost of the original PR while significantly improving the APIs throughout. It also restructures the builtins to support sharding across several independent tables. This accomplishes two improvements from the original PR: 1) It improves the APIs used significantly. 2) When builtins are defined from different sources (like SVE vs MVE in AArch64), this allows each of them to build their own string table independently rather than having to merge the string tables and info structures. 3) It allows each shard to factor out a common prefix, often cutting the size of the strings needed for the builtins by a factor two. The second point is important both to allow different mechanisms of construction (for example a `.def` file and a tablegen'ed `.inc` file, or different tablegen'ed `.inc files), it also simply reduces the sizes of these tables which is valuable given how large they are in some cases. The third builds on that size reduction. Initially, we use this new sharding rather than merging tables in AArch64, LoongArch, RISCV, and X86. Mostly this helps ensure the system works, as without further changes these still push scaling limits. Subsequent commits will more deeply leverage the new structure, including using the prefix capabilities which cannot be easily factored out here and requires deep changes to the targets.
…leGen This lets the TableGen-ed code be much cleaner, directly building an efficient string table without duplicates and without the repeated prefix.
… tables This leverages the sharded structure of the builtins to make it easy to directly tablegen most of the AArch64 and ARM builtins while still using X-macros for a few edge cases. It also extracts common prefixes as part of that. This makes the string tables for these targets dramatically smaller. This is especially important as the SVE builtins represent (by far) the largest string table and largest builtin table across all the targets in Clang.
…and info This moves the main builtins and several targets to use nice generated string tables and info structures rather than X-macros. Even without obvious prefixes factored out, the resulting tables are significantly smaller and much cheaper to compile with out all the X-macro overhead. This leaves the X-macros in place for atomic builtins which have a wide range of uses that don't seem reasonable to fold into TableGen. As future work, these should move to their own file (whether as X-macros or just generated patterns) so the AST headers don't have to include all the data for other builtins.
This requires adding support to the general builtins emission for producing prefixed builtin infos separately from un-prefixed which is a bit crufty. But we don't currently have any good way of having a more refined model than a single hard-coded prefix string per TableGen emission. Something more powerful and/or elegant is possible, but this is a fairly minimal first step that at least allows factoring out the builtin prefix for something like X86.
This target's builtins have an especially long prefix and so we get over 2x reduction in string table size required with this change.
chandlerc
force-pushed
the
prefix-arm-rv
branch
from
48fa9e0 to
6bedde2
Compare
I believe it's not me, though I don't like the changes at the beginning either. Anyway, with @chandlerc 's great script, we have migrated our internal bultins. Hope this won't affect them much. |
I would land it as the six-patch stack in this case. I ended up using that to review, since mainly the first patch is the interesting one and the target-specific migration ones are more mechanical and more incremental. |
SGTM, I'll rebase again and merge manually. Thanks again for the review! |
|
If you want to backport this, you need to add the PR to the milestone and comment
Then the bot should open a PR (or it'll tell you if it failed to cherry-pick, and how to proceed from there). |
Error: Command failed due to missing milestone. |
This is primarily to get commits involved in llvm/llvm-project#120534
This PR is for a patch series of 6 patches to try and fully address the Clang builtin string tables. I'm posting it as a single PR to give context to the series and explain what the overarching goal is, but each of the 6 patches is designed to be reasonable to review on its own and land on its own. However, intermediate states may push MSVC's compile time and memory usage due to especially large string tables.
The overarching goal of this patch series is to re-land the port of Clang's builtin tables to use offsets into string tables rather than containing dynamically relocated pointers to individual strings. A naive port as in #118734 has some serious issues due to forming extraordinarily large string tables, and many of them. To address this, subsequent patches restructure the builtin tables to make them smaller through sharding, use of TableGen-ed string tables without duplicates, and extracting a common prefix that can be dynamically added. The first patch results in a string table over 500KiB, but at the end the largest is well below 200KiB.
Combined, this patch series reduces the readonly data used by Clang's builtin tables by roughly 50% in addition to removing the dynamic relocations needed. The net result is a 30% reduction in the binary's dynamic relocation metadata, and a 2% reduction in the total binary size.
An overview of the various patches in the series:
Each patch has a more detailed description in its commit message.