Switch builtin strings to use string tables #118734
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-loongarch @llvm/pr-subscribers-backend-arm Author: Chandler Carruth (chandlerc) ChangesThe Clang binary (and any binary linking Clang as a library), when built using PIE, ends up with a pretty shocking number of dynamic relocations to apply to the executable image: roughly 400k. Each of these takes up binary space in the executable, and perhaps most interestingly takes start-up time to apply the relocations. The largest pattern I identified were the strings used to describe target builtins. The addresses of these string literals were stored into huge arrays, each one requiring a dynamic relocation. The way to avoid this is to design the target builtins to use a single large table of strings and offsets within the table for the individual strings. This switches the builtin management to such a scheme. This saves over 100k dynamic relocations by my measurement, an over 25% reduction. Just looking at byte size improvements, using the We get a 16% reduction in the This is also visible in my benchmarking of binary start-up overhead at least: We get about 2ms faster This PR implements the string tables using I was also able to find a reasonably clean and effective way of doing this with the existing macros and some There is one challenge with this approach: it requires the host compiler to support (very) long string literals, a bit over half a meg. =/ The current minimum MSVC version rejects these, but the very next patch release (16.8) removed that restriction. I'm going to send out a separate PR / RFC to raise the minimum version by one patch release, which I hope is acceptable as the current version was set years ago. FWIW, there are a few more low-hanging fruit sources of excessive dynamic relocations, maybe as many as 50k to 100k more that I'll take a look at to see if I can identify easy fixes. Beyond that, it seems to get quite difficult. It might be worth adding some guidance to developer documentation to try to avoid creating global data structures that repeatedly store pointers to other globals. Patch is 77.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/118734.diff 48 Files Affected:
diff --git a/clang/include/clang/Basic/Builtins.h b/clang/include/clang/Basic/Builtins.h
index 89f65682ae5b41..47729456380c43 100644
--- a/clang/include/clang/Basic/Builtins.h
+++ b/clang/include/clang/Basic/Builtins.h
@@ -55,6 +55,7 @@ struct HeaderDesc {
#undef HEADER
} ID;
+ constexpr HeaderDesc() : ID() {}
constexpr HeaderDesc(HeaderID ID) : ID(ID) {}
const char *getName() const;
@@ -68,14 +69,144 @@ enum ID {
FirstTSBuiltin
};
+// 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 {
+ int Name;
+ int Type;
+ int Attributes;
+ int Features;
+ } Offsets;
+
HeaderDesc Header;
LanguageID Langs;
};
+// The storage for `N` builtins. This contains a single pointer to the string
+// table used for these builtins and an array of metadata for each builtin.
+template <size_t N> struct Storage {
+ const char *StringTable;
+
+ std::array<Info, N> Infos;
+
+ // A constexpr function to construct the storage for a a given string table in
+ // the first argument and an array in the second argument. This is *only*
+ // expected to be used at compile time, we should mark it `consteval` when
+ // available.
+ //
+ // The `Infos` array is particularly special. This function expects an array
+ // of `Info` structs, where the string offsets of each entry refer to the
+ // *sizes* of those strings rather than their offsets, and for the target
+ // string to be in the provided string table at an offset the sum of all
+ // previous string sizes. This function walks the `Infos` array computing the
+ // running sum and replacing the sizes with the actual offsets in the string
+ // table that should be used. This arrangement is designed to make it easy to
+ // expand `.def` and `.inc` files with X-macros to construct both the string
+ // table and the `Info` structs in the arguments to this function.
+ static constexpr auto Make(const char *Strings,
+ std::array<Info, N> Infos) -> Storage<N> {
+ // Translate lengths to offsets.
+ int Offset = 0;
+ for (auto &I : Infos) {
+ Info::StrOffsets NewOffsets = {};
+ NewOffsets.Name = Offset;
+ Offset += I.Offsets.Name;
+ NewOffsets.Type = Offset;
+ Offset += I.Offsets.Type;
+ NewOffsets.Attributes = Offset;
+ Offset += I.Offsets.Attributes;
+ NewOffsets.Features = Offset;
+ Offset += I.Offsets.Features;
+ I.Offsets = NewOffsets;
+ }
+ return {Strings, Infos};
+ }
+};
+
+// 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
+
+// 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 `Storage::Make`.
+#define CLANG_BUILTIN_DETAIL_STR_OFFSETS(ID, TYPE, ATTRS) \
+ Builtin::Info::StrOffsets { \
+ llvm::StringLiteral(#ID).size() + 1, llvm::StringLiteral(TYPE).size() + 1, \
+ llvm::StringLiteral(ATTRS).size() + 1, \
+ llvm::StringLiteral("").size() + 1 \
+ }
+
+// 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 { \
+ llvm::StringLiteral(#ID).size() + 1, llvm::StringLiteral(TYPE).size() + 1, \
+ llvm::StringLiteral(ATTRS).size() + 1, \
+ llvm::StringLiteral(FEATURE).size() + 1 \
+ }
+
+// 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 Storage::Make 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,8 +214,11 @@ 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;
+ const char *TSStrTable = nullptr;
+ const char *AuxTSStrTable = nullptr;
+
+ llvm::ArrayRef<Info> TSInfos;
+ llvm::ArrayRef<Info> AuxTSInfos;
public:
Context() = default;
@@ -100,12 +234,13 @@ class Context {
/// Return the identifier name for the specified builtin,
/// e.g. "__builtin_abs".
- llvm::StringRef getName(unsigned ID) const { return getRecord(ID).Name; }
+ llvm::StringRef getName(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 {
@@ -114,40 +249,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
@@ -158,21 +293,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
@@ -180,7 +315,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
@@ -194,33 +329,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
@@ -245,27 +380,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 + TSInfos.size());
}
/// 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 - TSInfos.size(); }
/// Returns true if this is a libc/libm function without the '__builtin_'
/// prefix.
@@ -277,16 +410,21 @@ 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;
+ auto getStrTableAndInfo(unsigned ID) const
+ -> std::pair<const char *, const Info &>;
+
+ const Info &getInfo(unsigned ID) const {
+ return getStrTableAndInfo(ID).second;
+ }
/// Helper function for isPrintfLike and isScanfLike.
bool isLike(unsigned ID, unsigned &FormatIdx, bool &HasVAListArg,
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/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index 4420228793e95f..44fc0a08735f14 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -16,6 +16,7 @@
#include "clang/Basic/AddressSpaces.h"
#include "clang/Basic/BitmaskEnum.h"
+#include "clang/Basic/Builtins.h"
#include "clang/Basic/CFProtectionOptions.h"
#include "clang/Basic/CodeGenOptions.h"
#include "clang/Basic/LLVM.h"
@@ -1013,7 +1014,10 @@ class TargetInfo : public TransferrableTargetInfo,
/// Return information about target-specific builtins for
/// the current primary target, and info about which builtins are non-portable
/// across the current set of primary and secondary targets.
- virtual ArrayRef<Builtin::Info> getTargetBuiltins() const = 0;
+ virtual ArrayRef<Builtin::Info> getTargetBuiltins() const { return {}; };
+
+ virtual auto getTargetBuiltinStorage() const
+ -> std::pair<const char *, ArrayRef<Builtin::Info>> = 0;
/// Returns target-specific min and max values VScale_Range.
virtual std::optional<std::pair<unsigned, unsigned>>
diff --git a/clang/lib/Basic/Builtins.cpp b/clang/lib/Basic/Builtins.cpp
index 25a601573698e7..d1137cb6b6f13a 100644
--- a/clang/lib/Basic/Builtins.cpp
+++ b/clang/lib/Basic/Builtins.cpp
@@ -29,54 +29,93 @@ const char *HeaderDesc::getName() const {
llvm_unreachable("Unknown HeaderDesc::HeaderID enum");
}
-static constexpr Builtin::Info BuiltinInfo[] = {
- {"not a builtin function", nullptr, nullptr, nullptr, HeaderDesc::NO_HEADER,
- ALL_LANGUAGES},
-#define BUILTIN(ID, TYPE, ATTRS) \
- {#ID, TYPE, ATTRS, nullptr, HeaderDesc::NO_HEADER, ALL_LANGUAGES},
-#define LANGBUILTIN(ID, TYPE, ATTRS, LANGS) \
- {#ID, TYPE, ATTRS, nullptr, HeaderDesc::NO_HEADER, LANGS},
-#define LIBBUILTIN(ID, TYPE, ATTRS, HEADER, LANGS) \
- {#ID, TYPE, ATTRS, nullptr, HeaderDesc::HEADER, LANGS},
+static constexpr auto BuiltinStorage =
+ Builtin::Storage<Builtin::FirstTSBuiltin>::Make(
+ CLANG_BUILTIN_STR_TABLE("not a builtin function", "", "")
+#define BUILTIN CLANG_BUILTIN_STR_TABLE
#include "clang/Basic/Builtins.inc"
-};
+ ,
+ {CLANG_BUILTIN_ENTRY("not a builtin function", "", "")
+#define BUILTIN CLANG_BUILTIN_ENTRY
+#define LANGBUILTIN CLANG_LANGBUILTIN_ENTRY
+#define LIBBUILTIN CLANG_LIBBUILTIN_ENTRY
+#include "clang/Basic/Builtins.inc"
+ });
-const Builtin::Info &Builtin::Context::getRecord(unsigned ID) const {
+auto Builtin::Context::getStrTableAndInfo(unsigned ID) const
+ -> std::pair<const char *, const Info &> {
if (ID < Builtin::FirstTSBuiltin)
- return BuiltinInfo[ID];
- assert(((ID - Builtin::FirstTSBuiltin) <
- (TSRecords.size() + AuxTSRecords.size())) &&
- "Invalid builtin ID!");
+ return {BuiltinStorage.StringTable, BuiltinStorage.Infos[ID]};
+ assert(
+ ((ID - Builtin::FirstTSBuiltin) < (TSInfos.size() + AuxTSInfos.size())) &&
+ "Invalid builtin ID!");
if (isAuxBuiltinID(ID))
- return AuxTSRecords[getAuxBuiltinID(ID) - Builtin::FirstTSBuiltin];
- return TSRecords[ID - Builtin::FirstTSBuiltin];
+ return {AuxTSStrTable,
+ AuxTSInfos[getAuxBuiltinID(ID) - Builtin::FirstTSBuiltin]};
+ return {TSStrTable, TSInfos[ID - Builtin::FirstTSBuiltin]};
+}
+
+static llvm::StringRef getStrFromTable(const char *StrTable, int Offset) {
+ return &StrTable[Offset];
+}
+
+/// Return the identifier name for the specified builtin,
+/// e.g. "__builtin_abs".
+llvm::StringRef Builtin::Context::getName(unsigned ID) const {
+ const auto &[StrTable, I] = getStrTableAndInfo(ID);
+ return getStrFromTable(StrTable, I.Offsets.Name);
+}
+
+const char *Builti...
[truncated]
|
|
@llvm/pr-subscribers-backend-risc-v Author: Chandler Carruth (chandlerc) ChangesThe Clang binary (and any binary linking Clang as a library), when built using PIE, ends up with a pretty shocking number of dynamic relocations to apply to the executable image: roughly 400k. Each of these takes up binary space in the executable, and perhaps most interestingly takes start-up time to apply the relocations. The largest pattern I identified were the strings used to describe target builtins. The addresses of these string literals were stored into huge arrays, each one requiring a dynamic relocation. The way to avoid this is to design the target builtins to use a single large table of strings and offsets within the table for the individual strings. This switches the builtin management to such a scheme. This saves over 100k dynamic relocations by my measurement, an over 25% reduction. Just looking at byte size improvements, using the We get a 16% reduction in the This is also visible in my benchmarking of binary start-up overhead at least: We get about 2ms faster This PR implements the string tables using I was also able to find a reasonably clean and effective way of doing this with the existing macros and some There is one challenge with this approach: it requires the host compiler to support (very) long string literals, a bit over half a meg. =/ The current minimum MSVC version rejects these, but the very next patch release (16.8) removed that restriction. I'm going to send out a separate PR / RFC to raise the minimum version by one patch release, which I hope is acceptable as the current version was set years ago. FWIW, there are a few more low-hanging fruit sources of excessive dynamic relocations, maybe as many as 50k to 100k more that I'll take a look at to see if I can identify easy fixes. Beyond that, it seems to get quite difficult. It might be worth adding some guidance to developer documentation to try to avoid creating global data structures that repeatedly store pointers to other globals. Patch is 77.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/118734.diff 48 Files Affected:
diff --git a/clang/include/clang/Basic/Builtins.h b/clang/include/clang/Basic/Builtins.h
index 89f65682ae5b41..47729456380c43 100644
--- a/clang/include/clang/Basic/Builtins.h
+++ b/clang/include/clang/Basic/Builtins.h
@@ -55,6 +55,7 @@ struct HeaderDesc {
#undef HEADER
} ID;
+ constexpr HeaderDesc() : ID() {}
constexpr HeaderDesc(HeaderID ID) : ID(ID) {}
const char *getName() const;
@@ -68,14 +69,144 @@ enum ID {
FirstTSBuiltin
};
+// 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 {
+ int Name;
+ int Type;
+ int Attributes;
+ int Features;
+ } Offsets;
+
HeaderDesc Header;
LanguageID Langs;
};
+// The storage for `N` builtins. This contains a single pointer to the string
+// table used for these builtins and an array of metadata for each builtin.
+template <size_t N> struct Storage {
+ const char *StringTable;
+
+ std::array<Info, N> Infos;
+
+ // A constexpr function to construct the storage for a a given string table in
+ // the first argument and an array in the second argument. This is *only*
+ // expected to be used at compile time, we should mark it `consteval` when
+ // available.
+ //
+ // The `Infos` array is particularly special. This function expects an array
+ // of `Info` structs, where the string offsets of each entry refer to the
+ // *sizes* of those strings rather than their offsets, and for the target
+ // string to be in the provided string table at an offset the sum of all
+ // previous string sizes. This function walks the `Infos` array computing the
+ // running sum and replacing the sizes with the actual offsets in the string
+ // table that should be used. This arrangement is designed to make it easy to
+ // expand `.def` and `.inc` files with X-macros to construct both the string
+ // table and the `Info` structs in the arguments to this function.
+ static constexpr auto Make(const char *Strings,
+ std::array<Info, N> Infos) -> Storage<N> {
+ // Translate lengths to offsets.
+ int Offset = 0;
+ for (auto &I : Infos) {
+ Info::StrOffsets NewOffsets = {};
+ NewOffsets.Name = Offset;
+ Offset += I.Offsets.Name;
+ NewOffsets.Type = Offset;
+ Offset += I.Offsets.Type;
+ NewOffsets.Attributes = Offset;
+ Offset += I.Offsets.Attributes;
+ NewOffsets.Features = Offset;
+ Offset += I.Offsets.Features;
+ I.Offsets = NewOffsets;
+ }
+ return {Strings, Infos};
+ }
+};
+
+// 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
+
+// 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 `Storage::Make`.
+#define CLANG_BUILTIN_DETAIL_STR_OFFSETS(ID, TYPE, ATTRS) \
+ Builtin::Info::StrOffsets { \
+ llvm::StringLiteral(#ID).size() + 1, llvm::StringLiteral(TYPE).size() + 1, \
+ llvm::StringLiteral(ATTRS).size() + 1, \
+ llvm::StringLiteral("").size() + 1 \
+ }
+
+// 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 { \
+ llvm::StringLiteral(#ID).size() + 1, llvm::StringLiteral(TYPE).size() + 1, \
+ llvm::StringLiteral(ATTRS).size() + 1, \
+ llvm::StringLiteral(FEATURE).size() + 1 \
+ }
+
+// 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 Storage::Make 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,8 +214,11 @@ 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;
+ const char *TSStrTable = nullptr;
+ const char *AuxTSStrTable = nullptr;
+
+ llvm::ArrayRef<Info> TSInfos;
+ llvm::ArrayRef<Info> AuxTSInfos;
public:
Context() = default;
@@ -100,12 +234,13 @@ class Context {
/// Return the identifier name for the specified builtin,
/// e.g. "__builtin_abs".
- llvm::StringRef getName(unsigned ID) const { return getRecord(ID).Name; }
+ llvm::StringRef getName(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 {
@@ -114,40 +249,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
@@ -158,21 +293,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
@@ -180,7 +315,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
@@ -194,33 +329,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
@@ -245,27 +380,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 + TSInfos.size());
}
/// 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 - TSInfos.size(); }
/// Returns true if this is a libc/libm function without the '__builtin_'
/// prefix.
@@ -277,16 +410,21 @@ 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;
+ auto getStrTableAndInfo(unsigned ID) const
+ -> std::pair<const char *, const Info &>;
+
+ const Info &getInfo(unsigned ID) const {
+ return getStrTableAndInfo(ID).second;
+ }
/// Helper function for isPrintfLike and isScanfLike.
bool isLike(unsigned ID, unsigned &FormatIdx, bool &HasVAListArg,
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/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index 4420228793e95f..44fc0a08735f14 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -16,6 +16,7 @@
#include "clang/Basic/AddressSpaces.h"
#include "clang/Basic/BitmaskEnum.h"
+#include "clang/Basic/Builtins.h"
#include "clang/Basic/CFProtectionOptions.h"
#include "clang/Basic/CodeGenOptions.h"
#include "clang/Basic/LLVM.h"
@@ -1013,7 +1014,10 @@ class TargetInfo : public TransferrableTargetInfo,
/// Return information about target-specific builtins for
/// the current primary target, and info about which builtins are non-portable
/// across the current set of primary and secondary targets.
- virtual ArrayRef<Builtin::Info> getTargetBuiltins() const = 0;
+ virtual ArrayRef<Builtin::Info> getTargetBuiltins() const { return {}; };
+
+ virtual auto getTargetBuiltinStorage() const
+ -> std::pair<const char *, ArrayRef<Builtin::Info>> = 0;
/// Returns target-specific min and max values VScale_Range.
virtual std::optional<std::pair<unsigned, unsigned>>
diff --git a/clang/lib/Basic/Builtins.cpp b/clang/lib/Basic/Builtins.cpp
index 25a601573698e7..d1137cb6b6f13a 100644
--- a/clang/lib/Basic/Builtins.cpp
+++ b/clang/lib/Basic/Builtins.cpp
@@ -29,54 +29,93 @@ const char *HeaderDesc::getName() const {
llvm_unreachable("Unknown HeaderDesc::HeaderID enum");
}
-static constexpr Builtin::Info BuiltinInfo[] = {
- {"not a builtin function", nullptr, nullptr, nullptr, HeaderDesc::NO_HEADER,
- ALL_LANGUAGES},
-#define BUILTIN(ID, TYPE, ATTRS) \
- {#ID, TYPE, ATTRS, nullptr, HeaderDesc::NO_HEADER, ALL_LANGUAGES},
-#define LANGBUILTIN(ID, TYPE, ATTRS, LANGS) \
- {#ID, TYPE, ATTRS, nullptr, HeaderDesc::NO_HEADER, LANGS},
-#define LIBBUILTIN(ID, TYPE, ATTRS, HEADER, LANGS) \
- {#ID, TYPE, ATTRS, nullptr, HeaderDesc::HEADER, LANGS},
+static constexpr auto BuiltinStorage =
+ Builtin::Storage<Builtin::FirstTSBuiltin>::Make(
+ CLANG_BUILTIN_STR_TABLE("not a builtin function", "", "")
+#define BUILTIN CLANG_BUILTIN_STR_TABLE
#include "clang/Basic/Builtins.inc"
-};
+ ,
+ {CLANG_BUILTIN_ENTRY("not a builtin function", "", "")
+#define BUILTIN CLANG_BUILTIN_ENTRY
+#define LANGBUILTIN CLANG_LANGBUILTIN_ENTRY
+#define LIBBUILTIN CLANG_LIBBUILTIN_ENTRY
+#include "clang/Basic/Builtins.inc"
+ });
-const Builtin::Info &Builtin::Context::getRecord(unsigned ID) const {
+auto Builtin::Context::getStrTableAndInfo(unsigned ID) const
+ -> std::pair<const char *, const Info &> {
if (ID < Builtin::FirstTSBuiltin)
- return BuiltinInfo[ID];
- assert(((ID - Builtin::FirstTSBuiltin) <
- (TSRecords.size() + AuxTSRecords.size())) &&
- "Invalid builtin ID!");
+ return {BuiltinStorage.StringTable, BuiltinStorage.Infos[ID]};
+ assert(
+ ((ID - Builtin::FirstTSBuiltin) < (TSInfos.size() + AuxTSInfos.size())) &&
+ "Invalid builtin ID!");
if (isAuxBuiltinID(ID))
- return AuxTSRecords[getAuxBuiltinID(ID) - Builtin::FirstTSBuiltin];
- return TSRecords[ID - Builtin::FirstTSBuiltin];
+ return {AuxTSStrTable,
+ AuxTSInfos[getAuxBuiltinID(ID) - Builtin::FirstTSBuiltin]};
+ return {TSStrTable, TSInfos[ID - Builtin::FirstTSBuiltin]};
+}
+
+static llvm::StringRef getStrFromTable(const char *StrTable, int Offset) {
+ return &StrTable[Offset];
+}
+
+/// Return the identifier name for the specified builtin,
+/// e.g. "__builtin_abs".
+llvm::StringRef Builtin::Context::getName(unsigned ID) const {
+ const auto &[StrTable, I] = getStrTableAndInfo(ID);
+ return getStrFromTable(StrTable, I.Offsets.Name);
+}
+
+const char *Builti...
[truncated]
|
|
@llvm/pr-subscribers-backend-sparc Author: Chandler Carruth (chandlerc) ChangesThe Clang binary (and any binary linking Clang as a library), when built using PIE, ends up with a pretty shocking number of dynamic relocations to apply to the executable image: roughly 400k. Each of these takes up binary space in the executable, and perhaps most interestingly takes start-up time to apply the relocations. The largest pattern I identified were the strings used to describe target builtins. The addresses of these string literals were stored into huge arrays, each one requiring a dynamic relocation. The way to avoid this is to design the target builtins to use a single large table of strings and offsets within the table for the individual strings. This switches the builtin management to such a scheme. This saves over 100k dynamic relocations by my measurement, an over 25% reduction. Just looking at byte size improvements, using the We get a 16% reduction in the This is also visible in my benchmarking of binary start-up overhead at least: We get about 2ms faster This PR implements the string tables using I was also able to find a reasonably clean and effective way of doing this with the existing macros and some There is one challenge with this approach: it requires the host compiler to support (very) long string literals, a bit over half a meg. =/ The current minimum MSVC version rejects these, but the very next patch release (16.8) removed that restriction. I'm going to send out a separate PR / RFC to raise the minimum version by one patch release, which I hope is acceptable as the current version was set years ago. FWIW, there are a few more low-hanging fruit sources of excessive dynamic relocations, maybe as many as 50k to 100k more that I'll take a look at to see if I can identify easy fixes. Beyond that, it seems to get quite difficult. It might be worth adding some guidance to developer documentation to try to avoid creating global data structures that repeatedly store pointers to other globals. Patch is 77.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/118734.diff 48 Files Affected:
diff --git a/clang/include/clang/Basic/Builtins.h b/clang/include/clang/Basic/Builtins.h
index 89f65682ae5b41..47729456380c43 100644
--- a/clang/include/clang/Basic/Builtins.h
+++ b/clang/include/clang/Basic/Builtins.h
@@ -55,6 +55,7 @@ struct HeaderDesc {
#undef HEADER
} ID;
+ constexpr HeaderDesc() : ID() {}
constexpr HeaderDesc(HeaderID ID) : ID(ID) {}
const char *getName() const;
@@ -68,14 +69,144 @@ enum ID {
FirstTSBuiltin
};
+// 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 {
+ int Name;
+ int Type;
+ int Attributes;
+ int Features;
+ } Offsets;
+
HeaderDesc Header;
LanguageID Langs;
};
+// The storage for `N` builtins. This contains a single pointer to the string
+// table used for these builtins and an array of metadata for each builtin.
+template <size_t N> struct Storage {
+ const char *StringTable;
+
+ std::array<Info, N> Infos;
+
+ // A constexpr function to construct the storage for a a given string table in
+ // the first argument and an array in the second argument. This is *only*
+ // expected to be used at compile time, we should mark it `consteval` when
+ // available.
+ //
+ // The `Infos` array is particularly special. This function expects an array
+ // of `Info` structs, where the string offsets of each entry refer to the
+ // *sizes* of those strings rather than their offsets, and for the target
+ // string to be in the provided string table at an offset the sum of all
+ // previous string sizes. This function walks the `Infos` array computing the
+ // running sum and replacing the sizes with the actual offsets in the string
+ // table that should be used. This arrangement is designed to make it easy to
+ // expand `.def` and `.inc` files with X-macros to construct both the string
+ // table and the `Info` structs in the arguments to this function.
+ static constexpr auto Make(const char *Strings,
+ std::array<Info, N> Infos) -> Storage<N> {
+ // Translate lengths to offsets.
+ int Offset = 0;
+ for (auto &I : Infos) {
+ Info::StrOffsets NewOffsets = {};
+ NewOffsets.Name = Offset;
+ Offset += I.Offsets.Name;
+ NewOffsets.Type = Offset;
+ Offset += I.Offsets.Type;
+ NewOffsets.Attributes = Offset;
+ Offset += I.Offsets.Attributes;
+ NewOffsets.Features = Offset;
+ Offset += I.Offsets.Features;
+ I.Offsets = NewOffsets;
+ }
+ return {Strings, Infos};
+ }
+};
+
+// 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
+
+// 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 `Storage::Make`.
+#define CLANG_BUILTIN_DETAIL_STR_OFFSETS(ID, TYPE, ATTRS) \
+ Builtin::Info::StrOffsets { \
+ llvm::StringLiteral(#ID).size() + 1, llvm::StringLiteral(TYPE).size() + 1, \
+ llvm::StringLiteral(ATTRS).size() + 1, \
+ llvm::StringLiteral("").size() + 1 \
+ }
+
+// 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 { \
+ llvm::StringLiteral(#ID).size() + 1, llvm::StringLiteral(TYPE).size() + 1, \
+ llvm::StringLiteral(ATTRS).size() + 1, \
+ llvm::StringLiteral(FEATURE).size() + 1 \
+ }
+
+// 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 Storage::Make 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,8 +214,11 @@ 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;
+ const char *TSStrTable = nullptr;
+ const char *AuxTSStrTable = nullptr;
+
+ llvm::ArrayRef<Info> TSInfos;
+ llvm::ArrayRef<Info> AuxTSInfos;
public:
Context() = default;
@@ -100,12 +234,13 @@ class Context {
/// Return the identifier name for the specified builtin,
/// e.g. "__builtin_abs".
- llvm::StringRef getName(unsigned ID) const { return getRecord(ID).Name; }
+ llvm::StringRef getName(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 {
@@ -114,40 +249,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
@@ -158,21 +293,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
@@ -180,7 +315,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
@@ -194,33 +329,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
@@ -245,27 +380,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 + TSInfos.size());
}
/// 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 - TSInfos.size(); }
/// Returns true if this is a libc/libm function without the '__builtin_'
/// prefix.
@@ -277,16 +410,21 @@ 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;
+ auto getStrTableAndInfo(unsigned ID) const
+ -> std::pair<const char *, const Info &>;
+
+ const Info &getInfo(unsigned ID) const {
+ return getStrTableAndInfo(ID).second;
+ }
/// Helper function for isPrintfLike and isScanfLike.
bool isLike(unsigned ID, unsigned &FormatIdx, bool &HasVAListArg,
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/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index 4420228793e95f..44fc0a08735f14 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -16,6 +16,7 @@
#include "clang/Basic/AddressSpaces.h"
#include "clang/Basic/BitmaskEnum.h"
+#include "clang/Basic/Builtins.h"
#include "clang/Basic/CFProtectionOptions.h"
#include "clang/Basic/CodeGenOptions.h"
#include "clang/Basic/LLVM.h"
@@ -1013,7 +1014,10 @@ class TargetInfo : public TransferrableTargetInfo,
/// Return information about target-specific builtins for
/// the current primary target, and info about which builtins are non-portable
/// across the current set of primary and secondary targets.
- virtual ArrayRef<Builtin::Info> getTargetBuiltins() const = 0;
+ virtual ArrayRef<Builtin::Info> getTargetBuiltins() const { return {}; };
+
+ virtual auto getTargetBuiltinStorage() const
+ -> std::pair<const char *, ArrayRef<Builtin::Info>> = 0;
/// Returns target-specific min and max values VScale_Range.
virtual std::optional<std::pair<unsigned, unsigned>>
diff --git a/clang/lib/Basic/Builtins.cpp b/clang/lib/Basic/Builtins.cpp
index 25a601573698e7..d1137cb6b6f13a 100644
--- a/clang/lib/Basic/Builtins.cpp
+++ b/clang/lib/Basic/Builtins.cpp
@@ -29,54 +29,93 @@ const char *HeaderDesc::getName() const {
llvm_unreachable("Unknown HeaderDesc::HeaderID enum");
}
-static constexpr Builtin::Info BuiltinInfo[] = {
- {"not a builtin function", nullptr, nullptr, nullptr, HeaderDesc::NO_HEADER,
- ALL_LANGUAGES},
-#define BUILTIN(ID, TYPE, ATTRS) \
- {#ID, TYPE, ATTRS, nullptr, HeaderDesc::NO_HEADER, ALL_LANGUAGES},
-#define LANGBUILTIN(ID, TYPE, ATTRS, LANGS) \
- {#ID, TYPE, ATTRS, nullptr, HeaderDesc::NO_HEADER, LANGS},
-#define LIBBUILTIN(ID, TYPE, ATTRS, HEADER, LANGS) \
- {#ID, TYPE, ATTRS, nullptr, HeaderDesc::HEADER, LANGS},
+static constexpr auto BuiltinStorage =
+ Builtin::Storage<Builtin::FirstTSBuiltin>::Make(
+ CLANG_BUILTIN_STR_TABLE("not a builtin function", "", "")
+#define BUILTIN CLANG_BUILTIN_STR_TABLE
#include "clang/Basic/Builtins.inc"
-};
+ ,
+ {CLANG_BUILTIN_ENTRY("not a builtin function", "", "")
+#define BUILTIN CLANG_BUILTIN_ENTRY
+#define LANGBUILTIN CLANG_LANGBUILTIN_ENTRY
+#define LIBBUILTIN CLANG_LIBBUILTIN_ENTRY
+#include "clang/Basic/Builtins.inc"
+ });
-const Builtin::Info &Builtin::Context::getRecord(unsigned ID) const {
+auto Builtin::Context::getStrTableAndInfo(unsigned ID) const
+ -> std::pair<const char *, const Info &> {
if (ID < Builtin::FirstTSBuiltin)
- return BuiltinInfo[ID];
- assert(((ID - Builtin::FirstTSBuiltin) <
- (TSRecords.size() + AuxTSRecords.size())) &&
- "Invalid builtin ID!");
+ return {BuiltinStorage.StringTable, BuiltinStorage.Infos[ID]};
+ assert(
+ ((ID - Builtin::FirstTSBuiltin) < (TSInfos.size() + AuxTSInfos.size())) &&
+ "Invalid builtin ID!");
if (isAuxBuiltinID(ID))
- return AuxTSRecords[getAuxBuiltinID(ID) - Builtin::FirstTSBuiltin];
- return TSRecords[ID - Builtin::FirstTSBuiltin];
+ return {AuxTSStrTable,
+ AuxTSInfos[getAuxBuiltinID(ID) - Builtin::FirstTSBuiltin]};
+ return {TSStrTable, TSInfos[ID - Builtin::FirstTSBuiltin]};
+}
+
+static llvm::StringRef getStrFromTable(const char *StrTable, int Offset) {
+ return &StrTable[Offset];
+}
+
+/// Return the identifier name for the specified builtin,
+/// e.g. "__builtin_abs".
+llvm::StringRef Builtin::Context::getName(unsigned ID) const {
+ const auto &[StrTable, I] = getStrTableAndInfo(ID);
+ return getStrFromTable(StrTable, I.Offsets.Name);
+}
+
+const char *Builti...
[truncated]
|
|
@llvm/pr-subscribers-backend-msp430 Author: Chandler Carruth (chandlerc) ChangesThe Clang binary (and any binary linking Clang as a library), when built using PIE, ends up with a pretty shocking number of dynamic relocations to apply to the executable image: roughly 400k. Each of these takes up binary space in the executable, and perhaps most interestingly takes start-up time to apply the relocations. The largest pattern I identified were the strings used to describe target builtins. The addresses of these string literals were stored into huge arrays, each one requiring a dynamic relocation. The way to avoid this is to design the target builtins to use a single large table of strings and offsets within the table for the individual strings. This switches the builtin management to such a scheme. This saves over 100k dynamic relocations by my measurement, an over 25% reduction. Just looking at byte size improvements, using the We get a 16% reduction in the This is also visible in my benchmarking of binary start-up overhead at least: We get about 2ms faster This PR implements the string tables using I was also able to find a reasonably clean and effective way of doing this with the existing macros and some There is one challenge with this approach: it requires the host compiler to support (very) long string literals, a bit over half a meg. =/ The current minimum MSVC version rejects these, but the very next patch release (16.8) removed that restriction. I'm going to send out a separate PR / RFC to raise the minimum version by one patch release, which I hope is acceptable as the current version was set years ago. FWIW, there are a few more low-hanging fruit sources of excessive dynamic relocations, maybe as many as 50k to 100k more that I'll take a look at to see if I can identify easy fixes. Beyond that, it seems to get quite difficult. It might be worth adding some guidance to developer documentation to try to avoid creating global data structures that repeatedly store pointers to other globals. Patch is 77.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/118734.diff 48 Files Affected:
diff --git a/clang/include/clang/Basic/Builtins.h b/clang/include/clang/Basic/Builtins.h
index 89f65682ae5b41..47729456380c43 100644
--- a/clang/include/clang/Basic/Builtins.h
+++ b/clang/include/clang/Basic/Builtins.h
@@ -55,6 +55,7 @@ struct HeaderDesc {
#undef HEADER
} ID;
+ constexpr HeaderDesc() : ID() {}
constexpr HeaderDesc(HeaderID ID) : ID(ID) {}
const char *getName() const;
@@ -68,14 +69,144 @@ enum ID {
FirstTSBuiltin
};
+// 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 {
+ int Name;
+ int Type;
+ int Attributes;
+ int Features;
+ } Offsets;
+
HeaderDesc Header;
LanguageID Langs;
};
+// The storage for `N` builtins. This contains a single pointer to the string
+// table used for these builtins and an array of metadata for each builtin.
+template <size_t N> struct Storage {
+ const char *StringTable;
+
+ std::array<Info, N> Infos;
+
+ // A constexpr function to construct the storage for a a given string table in
+ // the first argument and an array in the second argument. This is *only*
+ // expected to be used at compile time, we should mark it `consteval` when
+ // available.
+ //
+ // The `Infos` array is particularly special. This function expects an array
+ // of `Info` structs, where the string offsets of each entry refer to the
+ // *sizes* of those strings rather than their offsets, and for the target
+ // string to be in the provided string table at an offset the sum of all
+ // previous string sizes. This function walks the `Infos` array computing the
+ // running sum and replacing the sizes with the actual offsets in the string
+ // table that should be used. This arrangement is designed to make it easy to
+ // expand `.def` and `.inc` files with X-macros to construct both the string
+ // table and the `Info` structs in the arguments to this function.
+ static constexpr auto Make(const char *Strings,
+ std::array<Info, N> Infos) -> Storage<N> {
+ // Translate lengths to offsets.
+ int Offset = 0;
+ for (auto &I : Infos) {
+ Info::StrOffsets NewOffsets = {};
+ NewOffsets.Name = Offset;
+ Offset += I.Offsets.Name;
+ NewOffsets.Type = Offset;
+ Offset += I.Offsets.Type;
+ NewOffsets.Attributes = Offset;
+ Offset += I.Offsets.Attributes;
+ NewOffsets.Features = Offset;
+ Offset += I.Offsets.Features;
+ I.Offsets = NewOffsets;
+ }
+ return {Strings, Infos};
+ }
+};
+
+// 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
+
+// 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 `Storage::Make`.
+#define CLANG_BUILTIN_DETAIL_STR_OFFSETS(ID, TYPE, ATTRS) \
+ Builtin::Info::StrOffsets { \
+ llvm::StringLiteral(#ID).size() + 1, llvm::StringLiteral(TYPE).size() + 1, \
+ llvm::StringLiteral(ATTRS).size() + 1, \
+ llvm::StringLiteral("").size() + 1 \
+ }
+
+// 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 { \
+ llvm::StringLiteral(#ID).size() + 1, llvm::StringLiteral(TYPE).size() + 1, \
+ llvm::StringLiteral(ATTRS).size() + 1, \
+ llvm::StringLiteral(FEATURE).size() + 1 \
+ }
+
+// 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 Storage::Make 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,8 +214,11 @@ 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;
+ const char *TSStrTable = nullptr;
+ const char *AuxTSStrTable = nullptr;
+
+ llvm::ArrayRef<Info> TSInfos;
+ llvm::ArrayRef<Info> AuxTSInfos;
public:
Context() = default;
@@ -100,12 +234,13 @@ class Context {
/// Return the identifier name for the specified builtin,
/// e.g. "__builtin_abs".
- llvm::StringRef getName(unsigned ID) const { return getRecord(ID).Name; }
+ llvm::StringRef getName(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 {
@@ -114,40 +249,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
@@ -158,21 +293,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
@@ -180,7 +315,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
@@ -194,33 +329,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
@@ -245,27 +380,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 + TSInfos.size());
}
/// 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 - TSInfos.size(); }
/// Returns true if this is a libc/libm function without the '__builtin_'
/// prefix.
@@ -277,16 +410,21 @@ 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;
+ auto getStrTableAndInfo(unsigned ID) const
+ -> std::pair<const char *, const Info &>;
+
+ const Info &getInfo(unsigned ID) const {
+ return getStrTableAndInfo(ID).second;
+ }
/// Helper function for isPrintfLike and isScanfLike.
bool isLike(unsigned ID, unsigned &FormatIdx, bool &HasVAListArg,
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/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index 4420228793e95f..44fc0a08735f14 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -16,6 +16,7 @@
#include "clang/Basic/AddressSpaces.h"
#include "clang/Basic/BitmaskEnum.h"
+#include "clang/Basic/Builtins.h"
#include "clang/Basic/CFProtectionOptions.h"
#include "clang/Basic/CodeGenOptions.h"
#include "clang/Basic/LLVM.h"
@@ -1013,7 +1014,10 @@ class TargetInfo : public TransferrableTargetInfo,
/// Return information about target-specific builtins for
/// the current primary target, and info about which builtins are non-portable
/// across the current set of primary and secondary targets.
- virtual ArrayRef<Builtin::Info> getTargetBuiltins() const = 0;
+ virtual ArrayRef<Builtin::Info> getTargetBuiltins() const { return {}; };
+
+ virtual auto getTargetBuiltinStorage() const
+ -> std::pair<const char *, ArrayRef<Builtin::Info>> = 0;
/// Returns target-specific min and max values VScale_Range.
virtual std::optional<std::pair<unsigned, unsigned>>
diff --git a/clang/lib/Basic/Builtins.cpp b/clang/lib/Basic/Builtins.cpp
index 25a601573698e7..d1137cb6b6f13a 100644
--- a/clang/lib/Basic/Builtins.cpp
+++ b/clang/lib/Basic/Builtins.cpp
@@ -29,54 +29,93 @@ const char *HeaderDesc::getName() const {
llvm_unreachable("Unknown HeaderDesc::HeaderID enum");
}
-static constexpr Builtin::Info BuiltinInfo[] = {
- {"not a builtin function", nullptr, nullptr, nullptr, HeaderDesc::NO_HEADER,
- ALL_LANGUAGES},
-#define BUILTIN(ID, TYPE, ATTRS) \
- {#ID, TYPE, ATTRS, nullptr, HeaderDesc::NO_HEADER, ALL_LANGUAGES},
-#define LANGBUILTIN(ID, TYPE, ATTRS, LANGS) \
- {#ID, TYPE, ATTRS, nullptr, HeaderDesc::NO_HEADER, LANGS},
-#define LIBBUILTIN(ID, TYPE, ATTRS, HEADER, LANGS) \
- {#ID, TYPE, ATTRS, nullptr, HeaderDesc::HEADER, LANGS},
+static constexpr auto BuiltinStorage =
+ Builtin::Storage<Builtin::FirstTSBuiltin>::Make(
+ CLANG_BUILTIN_STR_TABLE("not a builtin function", "", "")
+#define BUILTIN CLANG_BUILTIN_STR_TABLE
#include "clang/Basic/Builtins.inc"
-};
+ ,
+ {CLANG_BUILTIN_ENTRY("not a builtin function", "", "")
+#define BUILTIN CLANG_BUILTIN_ENTRY
+#define LANGBUILTIN CLANG_LANGBUILTIN_ENTRY
+#define LIBBUILTIN CLANG_LIBBUILTIN_ENTRY
+#include "clang/Basic/Builtins.inc"
+ });
-const Builtin::Info &Builtin::Context::getRecord(unsigned ID) const {
+auto Builtin::Context::getStrTableAndInfo(unsigned ID) const
+ -> std::pair<const char *, const Info &> {
if (ID < Builtin::FirstTSBuiltin)
- return BuiltinInfo[ID];
- assert(((ID - Builtin::FirstTSBuiltin) <
- (TSRecords.size() + AuxTSRecords.size())) &&
- "Invalid builtin ID!");
+ return {BuiltinStorage.StringTable, BuiltinStorage.Infos[ID]};
+ assert(
+ ((ID - Builtin::FirstTSBuiltin) < (TSInfos.size() + AuxTSInfos.size())) &&
+ "Invalid builtin ID!");
if (isAuxBuiltinID(ID))
- return AuxTSRecords[getAuxBuiltinID(ID) - Builtin::FirstTSBuiltin];
- return TSRecords[ID - Builtin::FirstTSBuiltin];
+ return {AuxTSStrTable,
+ AuxTSInfos[getAuxBuiltinID(ID) - Builtin::FirstTSBuiltin]};
+ return {TSStrTable, TSInfos[ID - Builtin::FirstTSBuiltin]};
+}
+
+static llvm::StringRef getStrFromTable(const char *StrTable, int Offset) {
+ return &StrTable[Offset];
+}
+
+/// Return the identifier name for the specified builtin,
+/// e.g. "__builtin_abs".
+llvm::StringRef Builtin::Context::getName(unsigned ID) const {
+ const auto &[StrTable, I] = getStrTableAndInfo(ID);
+ return getStrFromTable(StrTable, I.Offsets.Name);
+}
+
+const char *Builti...
[truncated]
|
|
Discussion thread about the MSVC version change: https://discourse.llvm.org/t/rfc-raising-minimum-msvc-version-by-one-patch-release/83490 |
There was a problem hiding this comment.
This looks fine to me - though given the broad impact (to every target in clang) maybe @AaronBallman wants to weigh in.
If you wanted to front-load the things like getRecord(ID).Attributes -> getAttributesString(ID) those things could possibly be done in advance.
There was a problem hiding this comment.
LGTM too. I do wonder if we can make tablegen generate the data directly in the desired format here (perhaps with some additional .td files to define exactly which files we want to include in which targets and to define the placeholder for the target-independent builtins), but I don't think this (huge) improvement needs to block on that.
|
Updated to use the GCC diagnostic push the same as TableGen does for long string literals. Also added Aaron to take a look as well (unless he's comfortable already). |
This comment was marked as outdated.
This comment was marked as outdated.
Hmm... maybe it is ccache. Let me try doing a build with an empty cache and see if it still fails, otherwise, I'll extract that file. |
A clean build with an empty ccache does not seem to have made a difference. :( I've pulled out the entire |
Can you put it in a zip somewhere? It would be interesting to check if there's some major difference compared to using a different MSVC version. |
I've put it up on Dropbox, let me know if you cannot access the zipfile: https://www.dropbox.com/scl/fi/pbc02zyba2wtcwhun27fz/Basic.zip?rlkey=v4vqd6bqoewubmkzca8508wxo&dl=0 |
I looked at Builtins.cpp.obj and compared to the one in my build dir. They're very similar, in particular the string table is there in both files, and looks identical. I don't really have any more ideas for debugging this. |
|
No real updates here, but our internal builder did catch up to this commit and we are seeing the same (and a lot more) failures when this commit is merged into our downstream codebase. I was kind of hoping that it would pass so that it might indicate that the problem might be a configuration issue with our upstream build bot, but the fact that it fails in a similar manner likely points to some kind of bug in the version of MSVC that we are using. @chandlerc could we consider reverting the change until we can figure out what the problem is? I will continue to take a look into it, but I don't really have any ideas what might be the cause at the moment, so have no ETA on a work-around, and in the meantime, this will continue to cause test failures for us internally. |
It really would be helpful to try a different version of MSVC. As I pointed out in the original reply to the build bot failure, this bot seems to be using a version of MSVC that isn't even a listed release. The last patch release of 16.9 is Would it help for me to post a PR that adds some validation of the string table? I can easily do that, and maybe that would let you isolate whether this is definitely an MSVC version issue vs. something with the code.
I'm happy to revert, and I'll start working on that. But I think there needs to be a concrete ETA on being able to make progress. Especially given the fact that no other builders seem to hit this and it may be specific to an MSVC version that even other windows devs don't realistically have access to... Can you give some fairly concrete timeline? 1 week? maybe 2? Otherwise, there is no actual path to resolving this. |
This reverts commit be2df95.
The build was released as far as I'm aware (we don't install pre-release versions on our production machines). That being said, I'm guessing it is probably a bug with the specific version of MSVC that we are using. I have started the conversation internally about upgrading the version of MSVC we use to build, but that will take time.
Sure, I'm happy to try anything that you think might help and this is one of my higher priority issues at the moment.
I will look into it the rest of this week and into next week. I don't really know the problem here, so I cannot promise a resolution date, but I will try my best to get this resolved, it is making our internal automation fail, so I want to unblock that! |
|
So, the builder came back up, and is now passing without this landing... Here is the first build after it came back up: https://lab.llvm.org/buildbot/#/builders/46/builds/9173 And I went through the output and found a specific test that was failing, and it seems to pass? It looks like the builder is now using MSVC version 14.29.30133 -- so that might explain it. This maybe means that there is some issue with later patch releases of 14.28 (in Visual Studio v16.9 as I understand it)? Are all of your builders OK moving to 14.29 (Visual Studio 16.11 from my reading)? |
That was a mistake. I originally was going to update the build bot to a later version of Visual Studio as while it wouldn't match our internal configuration, it was still useful to have some Windows coverage of the components we build/test. I then changed my mind and was just going to restore the original configuration (and thought I had) but had accidentally replaced the compiler with a newer version. I'm going to put the original one back. I have started the process to try and upgrade the version we are using internally, but that will likely take a while. |
Ah, no problem. Let me know when its up and I can wait for at least one build to cycle before reverting.
Of course, that's part of why I asked -- I was quite surprised to see the update. This does at least seem to isolate to MSVC version though and not some other aspect of the machine or configuration, which is actually really useful to know. |
|
I see the build bot up and with the original failure, going to merge the revert to hopefully make sure it goes green after that. |
Reverts #118734 There are currently some specific versions of MSVC that are miscompiling this code (we think). We don't know why as all the other build bots and at least some folks' local Windows builds work fine. This is a candidate revert to help the relevant folks catch their builders up and have time to debug the issue. However, the expectation is to roll forward at some point with a workaround if at all possible.
This reverts commit ca79ff0. It also updates the original PR to use 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.
This reverts commit ca79ff0. It also updates the original PR to use 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.
This reverts commit ca79ff0. It also updates the original PR to use 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.
This reverts commit ca79ff0. It also updates the original PR to use 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.
This reverts commit ca79ff0. It also updates the original PR to use 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.
This reverts commit ca79ff0. It also updates the original PR to use 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.
This reverts commit ca79ff0. It also updates the original PR to use 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.
This reverts commit ca79ff0. It also updates the original PR to use 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.
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.
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.
This both reapplies #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.
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.
The Clang binary (and any binary linking Clang as a library), when built using PIE, ends up with a pretty shocking number of dynamic relocations to apply to the executable image: roughly 400k.
Each of these takes up binary space in the executable, and perhaps most interestingly takes start-up time to apply the relocations.
The largest pattern I identified were the strings used to describe target builtins. The addresses of these string literals were stored into huge arrays, each one requiring a dynamic relocation. The way to avoid this is to design the target builtins to use a single large table of strings and offsets within the table for the individual strings. This switches the builtin management to such a scheme.
This saves over 100k dynamic relocations by my measurement, an over 25% reduction. Just looking at byte size improvements, using the
bloatytool to compare a newly builtclangbinary to an old one:We get a 16% reduction in the
.data.rel.rosection, and nearly 30% reduction in.rela.dynwhere those reloctaions are stored.This is also visible in my benchmarking of binary start-up overhead at least:
We get about 2ms faster
--versionruns. While there is a lot of noise in binary execution time, this delta is pretty consistent, and represents over 10% improvement. This is particularly interesting to me because for very short source files, repeatedly starting theclangbinary is actually the dominant cost. For example,configurescripts running against theclangcompiler are slow in large part because of binary start up time, not the time to process the actual inputs to the compiler.This PR implements the string tables using
constexprcode and the existing macro system. I understand that the builtins are moving towards a TableGen model, and if complete that would provide more options for modeling this. Unfortunately, that migration isn't complete, and even the parts that are migrated still rely on the ability to break out of the TableGen model and directly expand an X-macro styleBUILTIN(...)textually. I looked at trying to complete the move to TableGen, but it would both require the difficult migration of the remaining targets, and solving some tricky problems with how to move away from any macro-based expansion.I was also able to find a reasonably clean and effective way of doing this with the existing macros and some
constexprcode that I think is clean enough to be a pretty good intermediate state, and maybe give a good target for the eventual TableGen solution. I was also able to factor the macros into set of consistent patterns that avoids a significant regression in overall boilerplate.There is one challenge with this approach: it requires the host compiler to support (very) long string literals, a bit over half a meg. =/ The current minimum MSVC version rejects these, but the very next patch release (16.8) removed that restriction. I'm going to send out a separate PR / RFC to raise the minimum version by one patch release, which I hope is acceptable as the current version was set years ago.
FWIW, there are a few more low-hanging fruit sources of excessive dynamic relocations, maybe as many as 50k to 100k more that I'll take a look at to see if I can identify easy fixes. Beyond that, it seems to get quite difficult. It might be worth adding some guidance to developer documentation to try to avoid creating global data structures that repeatedly store pointers to other globals.