[llvm-branch-commits] [llvm] release/19.x: [AArch64] Don't replace dst of SWP instructions with (X|W)ZR (#102139) (PR #102316)
https://github.com/statham-arm approved this pull request. Seems sensible to me. It's fixing a genuine codegen fault (a subtle one, but of course that makes it worse – harder to spot when it occurs!). And it's a small safe change that disables one very small case of a conceptually simple optimisation, unlikely to introduce other bugs. https://github.com/llvm/llvm-project/pull/102316 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] release/19.x: [AArch64] Don't replace dst of SWP instructions with (X|W)ZR (#102139) (PR #102316)
https://github.com/statham-arm edited https://github.com/llvm/llvm-project/pull/102316 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [clang] [Multilib] Custom flags processing for library selection (PR #110659)
@@ -95,9 +96,113 @@ MultilibSet &MultilibSet::FilterOut(FilterCallback F) {
void MultilibSet::push_back(const Multilib &M) { Multilibs.push_back(M); }
+static void WarnUnclaimedMultilibCustomFlags(
+const Driver &D, const SmallVector &UnclaimedCustomFlagValues,
+const SmallVector &CustomFlagDecls)
{
+ struct EditDistanceInfo {
+StringRef FlagValue;
+unsigned EditDistance;
+ };
+ const unsigned MaxEditDistance = 5;
+
+ for (StringRef Unclaimed : UnclaimedCustomFlagValues) {
+std::optional BestCandidate;
+for (const auto &Decl : CustomFlagDecls) {
+ for (const auto &Value : Decl->ValueList) {
+const std::string &FlagValueName = Value.Name;
+unsigned EditDistance =
+Unclaimed.edit_distance(FlagValueName, /*AllowReplacements=*/true,
+/*MaxEditDistance=*/MaxEditDistance);
+if (!BestCandidate || (EditDistance <= MaxEditDistance &&
+ EditDistance < BestCandidate->EditDistance)) {
+ BestCandidate = {FlagValueName, EditDistance};
+}
+ }
+}
+if (!BestCandidate)
+ D.Diag(clang::diag::warn_drv_unsupported_opt)
+ << (custom_flag::Prefix + Unclaimed).str();
+else
+ D.Diag(clang::diag::warn_drv_unsupported_opt_with_suggestion)
+ << (custom_flag::Prefix + Unclaimed).str()
+ << (custom_flag::Prefix + BestCandidate->FlagValue).str();
+ }
+}
+
+namespace clang::driver::custom_flag {
+class ValueNameToDetailMap {
+ SmallVector> Mapping;
+
+public:
+ template
+ ValueNameToDetailMap(It FlagDeclsBegin, It FlagDeclsEnd) {
+for (auto DeclIt = FlagDeclsBegin; DeclIt != FlagDeclsEnd; ++DeclIt) {
+ const CustomFlagDeclarationPtr &Decl = *DeclIt;
+ for (const auto &Value : Decl->ValueList)
+Mapping.emplace_back(Value.Name, &Value);
+}
+ }
+
+ const CustomFlagValueDetail *get(StringRef Key) const {
+auto Iter = llvm::find_if(
+Mapping, [&](const auto &Pair) { return Pair.first == Key; });
+return Iter != Mapping.end() ? Iter->second : nullptr;
+ }
+};
+} // namespace clang::driver::custom_flag
+
+Multilib::flags_list
+MultilibSet::processCustomFlags(const Driver &D,
+const Multilib::flags_list &Flags) const {
+ Multilib::flags_list Result;
+ SmallVector
+ ClaimedCustomFlagValues;
+ SmallVector UnclaimedCustomFlagValueStrs;
+
+ const auto ValueNameToValueDetail = custom_flag::ValueNameToDetailMap(
+ CustomFlagDecls.begin(), CustomFlagDecls.end());
+
+ for (StringRef Flag : Flags) {
+if (!Flag.starts_with(custom_flag::Prefix)) {
+ Result.push_back(Flag.str());
+ continue;
+}
+
+StringRef CustomFlagValueStr = Flag.substr(custom_flag::Prefix.size());
+const custom_flag::CustomFlagValueDetail *Detail =
+ValueNameToValueDetail.get(CustomFlagValueStr);
+if (Detail)
+ ClaimedCustomFlagValues.push_back(Detail);
+else
+ UnclaimedCustomFlagValueStrs.push_back(CustomFlagValueStr);
+ }
+
+ llvm::SmallSet
+ TriggeredCustomFlagDecls;
+
+ for (auto *CustomFlagValue : llvm::reverse(ClaimedCustomFlagValues)) {
+if (!TriggeredCustomFlagDecls.insert(CustomFlagValue->Decl).second)
+ continue;
+Result.push_back(std::string(custom_flag::Prefix) + CustomFlagValue->Name);
+ }
statham-arm wrote:
Can you add some comments explaining what all this code is for, please?
It looks as if the first loop over `Flags` is picking out custom multilib
flags, taking off the `-fmultilib-flag=` prefix, and storing them in
`ClaimedCustomFlagValues`, and then this second loop is putting them back on to
the result string, after putting the `-fmultilib-flag=` prefix back on.
I'm sure there's a reason why this is _not_ an elaborate no-op, but can you
write down what the reason is? Why aren't we just putting the flags straight
into `Result` without taking them apart and putting them back together in
between? Without any comments it looks like a lot of processing for not much
change.
https://github.com/llvm/llvm-project/pull/110659
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [clang] [Multilib] Custom flags processing for library selection (PR #110659)
@@ -14,6 +14,12 @@ def err_drv_no_such_file_with_suggestion : Error< def err_drv_unsupported_opt : Error<"unsupported option '%0'">; def err_drv_unsupported_opt_with_suggestion : Error< "unsupported option '%0'; did you mean '%1'?">; +def warn_drv_unsupported_opt : Warning< + "unsupported option '%0'">, + InGroup; +def warn_drv_unsupported_opt_with_suggestion : Warning< + "unsupported option '%0'; did you mean '%1'?">, + InGroup; statham-arm wrote: You mention in the commit message that an unrecognised custom flag is a warning rather than an error. But I'm curious about why. If a toolchain vendor ships a toolchain with a collection of flags that select particular libraries, and a user misspells a flag in their makefile, why _wouldn't_ they want a build error so that they notice and fix it? A warning can easily be scrolled off the screen in the enormous build log. Warnings are for cases that _might_ be mistakes by the user, but might also be correct, so you can't afford to refuse to build. I don't see why an unrecognised flag falls into that category. It would make sense if there was some kind of standard flag that _most_ multilib collections agreed on, and you wanted to be able to use that flag everywhere and have it quietly ignored when not supported. But is that your intention? https://github.com/llvm/llvm-project/pull/110659 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [clang] [Multilib] Add -fmultilib-flag command-line option (PR #110658)
@@ -196,6 +196,16 @@ bool ToolChain::defaultToIEEELongDouble() const {
return PPC_LINUX_DEFAULT_IEEELONGDOUBLE && getTriple().isOSLinux();
}
+static void
+processARMAArch64MultilibCustomFlags(Multilib::flags_list &List,
statham-arm wrote:
Is this system intended to be permanently specific to Arm and AArch64? I don't
really see why it should be: the need to build libraries distinguished by a
configuration option that isn't a standard compile flag is certainly not
specific to Arm. Perhaps this should just be called
`processMultilibCustomFlags`?
https://github.com/llvm/llvm-project/pull/110658
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [compiler-rt] release/19.x: [compiler-rt] Stop using x86 builtin on AArch64 with GCC (#93890) (PR #115006)
https://github.com/statham-arm approved this pull request. Sounds like a good idea to me! https://github.com/llvm/llvm-project/pull/115006 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [clang] [Multilib] Add -fmultilib-flag command-line option (PR #110658)
https://github.com/statham-arm approved this pull request. https://github.com/llvm/llvm-project/pull/110658 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [clang] [Multilib] Custom flags processing for library selection (PR #110659)
@@ -95,9 +96,113 @@ MultilibSet &MultilibSet::FilterOut(FilterCallback F) {
void MultilibSet::push_back(const Multilib &M) { Multilibs.push_back(M); }
+static void WarnUnclaimedMultilibCustomFlags(
+const Driver &D, const SmallVector &UnclaimedCustomFlagValues,
+const SmallVector &CustomFlagDecls)
{
+ struct EditDistanceInfo {
+StringRef FlagValue;
+unsigned EditDistance;
+ };
+ const unsigned MaxEditDistance = 5;
+
+ for (StringRef Unclaimed : UnclaimedCustomFlagValues) {
+std::optional BestCandidate;
+for (const auto &Decl : CustomFlagDecls) {
+ for (const auto &Value : Decl->ValueList) {
+const std::string &FlagValueName = Value.Name;
+unsigned EditDistance =
+Unclaimed.edit_distance(FlagValueName, /*AllowReplacements=*/true,
+/*MaxEditDistance=*/MaxEditDistance);
+if (!BestCandidate || (EditDistance <= MaxEditDistance &&
+ EditDistance < BestCandidate->EditDistance)) {
+ BestCandidate = {FlagValueName, EditDistance};
+}
+ }
+}
+if (!BestCandidate)
+ D.Diag(clang::diag::warn_drv_unsupported_opt)
+ << (custom_flag::Prefix + Unclaimed).str();
+else
+ D.Diag(clang::diag::warn_drv_unsupported_opt_with_suggestion)
+ << (custom_flag::Prefix + Unclaimed).str()
+ << (custom_flag::Prefix + BestCandidate->FlagValue).str();
+ }
+}
+
+namespace clang::driver::custom_flag {
+class ValueNameToDetailMap {
+ SmallVector> Mapping;
+
+public:
+ template
+ ValueNameToDetailMap(It FlagDeclsBegin, It FlagDeclsEnd) {
+for (auto DeclIt = FlagDeclsBegin; DeclIt != FlagDeclsEnd; ++DeclIt) {
+ const CustomFlagDeclarationPtr &Decl = *DeclIt;
+ for (const auto &Value : Decl->ValueList)
+Mapping.emplace_back(Value.Name, &Value);
+}
+ }
+
+ const CustomFlagValueDetail *get(StringRef Key) const {
+auto Iter = llvm::find_if(
+Mapping, [&](const auto &Pair) { return Pair.first == Key; });
+return Iter != Mapping.end() ? Iter->second : nullptr;
+ }
+};
+} // namespace clang::driver::custom_flag
+
+Multilib::flags_list
+MultilibSet::processCustomFlags(const Driver &D,
+const Multilib::flags_list &Flags) const {
+ Multilib::flags_list Result;
+ SmallVector
+ ClaimedCustomFlagValues;
+ SmallVector UnclaimedCustomFlagValueStrs;
+
+ const auto ValueNameToValueDetail = custom_flag::ValueNameToDetailMap(
+ CustomFlagDecls.begin(), CustomFlagDecls.end());
+
+ for (StringRef Flag : Flags) {
+if (!Flag.starts_with(custom_flag::Prefix)) {
+ Result.push_back(Flag.str());
+ continue;
+}
+
+StringRef CustomFlagValueStr = Flag.substr(custom_flag::Prefix.size());
+const custom_flag::CustomFlagValueDetail *Detail =
+ValueNameToValueDetail.get(CustomFlagValueStr);
+if (Detail)
+ ClaimedCustomFlagValues.push_back(Detail);
+else
+ UnclaimedCustomFlagValueStrs.push_back(CustomFlagValueStr);
+ }
+
+ llvm::SmallSet
+ TriggeredCustomFlagDecls;
+
+ for (auto *CustomFlagValue : llvm::reverse(ClaimedCustomFlagValues)) {
+if (!TriggeredCustomFlagDecls.insert(CustomFlagValue->Decl).second)
+ continue;
+Result.push_back(std::string(custom_flag::Prefix) + CustomFlagValue->Name);
+ }
statham-arm wrote:
Thanks. Yes, that's better: the de-duplication wasn't at all obvious, and now
it is. (And I much prefer the explanation in comments in the code than here
where nobody will ever look for it :-)
https://github.com/llvm/llvm-project/pull/110659
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [clang] [Multilib] Custom flags processing for library selection (PR #110659)
https://github.com/statham-arm approved this pull request. https://github.com/llvm/llvm-project/pull/110659 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [compiler-rt] release/19.x: [compiler-rt] Stop using x86 builtin on AArch64 with GCC (#93890) (PR #115006)
statham-arm wrote: Despite the force-push, I think the current patch looks the same as the one I clicked approve on (maybe it was just rebased?). I can't see a problem – it's the same patch that hasn't been causing trouble on `main`. https://github.com/llvm/llvm-project/pull/115006 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [IR] "modular-format" attribute for functions using format strings (PR #147429)
@@ -2620,6 +2620,23 @@ For example: This attribute indicates that outlining passes should not modify the function. +``"modular_format"=""`` statham-arm wrote: Thanks. An afterthought: even printf and _scanf_ are different enough that they'd need separate handling, so I don't even need to postulate a hypothetical format string syntax from some other language's standard library. For example, your check for arguments of actual floating-point type is fine for telling that a printf call doesn't need float formatting, but for a scanf call, you'd need to look for _pointers_ to floating types instead. https://github.com/llvm/llvm-project/pull/147429 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [libc] [libc] Modular printf option (float only) (PR #147426)
@@ -0,0 +1,41 @@
+#ifdef LIBC_COPT_PRINTF_MODULAR
+#include "src/__support/arg_list.h"
+
+#define LIBC_PRINTF_DEFINE_MODULAR
+#include "src/stdio/printf_core/float_dec_converter.h"
+#include "src/stdio/printf_core/float_hex_converter.h"
+#include "src/stdio/printf_core/parser.h"
+
+namespace LIBC_NAMESPACE_DECL {
+namespace printf_core {
+template class Parser;
+template class Parser>;
+template class Parser>;
+template class Parser>;
+template class Parser>;
+
+#define INSTANTIATE_CONVERT_FN(NAME)
\
+ template int NAME(
\
+ Writer * writer,
\
+ const FormatSection &to_conv);
\
+ template int NAME(
\
+ Writer * writer,
\
+ const FormatSection &to_conv);
\
+ template int NAME(
\
+ Writer * writer,
\
+ const FormatSection &to_conv);
\
+ template int NAME(
\
+ Writer * writer,
\
+ const FormatSection &to_conv)
+
+INSTANTIATE_CONVERT_FN(convert_float_decimal);
+INSTANTIATE_CONVERT_FN(convert_float_dec_exp);
+INSTANTIATE_CONVERT_FN(convert_float_dec_auto);
+INSTANTIATE_CONVERT_FN(convert_float_hex_exp);
+
+} // namespace printf_core
+} // namespace LIBC_NAMESPACE_DECL
+
+// Bring this file into the link if __printf_float is referenced.
+extern "C" void __printf_float() {}
statham-arm wrote:
One risk with this strategy: what happens if this file is compiled with
`-ffunction-sections`? Then `__printf_float` might be in a different code
section from the template instantiations, and a linker might garbage-collect
the functions you actually wanted, treating the weak references to them as not
enough reason to keep them.
If I wanted to be sure of this technique, I'd either enforce via compile
options that everything in this file is in the same code section, or else add
further `BFD_RELOC_NONE` links from `__printf_float` to the payload functions,
to make sure there's an unbroken chain of non-weak references from the import
of `__printf_float` to the functions that actually do the work.
https://github.com/llvm/llvm-project/pull/147426
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [IR] "modular-format" attribute for functions using format strings (PR #147429)
@@ -2620,6 +2620,23 @@ For example: This attribute indicates that outlining passes should not modify the function. +``"modular_format"=""`` statham-arm wrote: Is this intended to be specific to _printf_ style format strings? Or will it potentially be reusable for other formatting languages? The text here just says "format string" without saying what kind. For futureproofing, perhaps there should be some kind of a type parameter here saying which formatting mechanism is referred to? That way if two incompatible formatting syntaxes need to be supported later, there's space for expansion. https://github.com/llvm/llvm-project/pull/147429 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [libc] [libc] Modular printf option (float only) (PR #147426)
@@ -0,0 +1,41 @@
+#ifdef LIBC_COPT_PRINTF_MODULAR
+#include "src/__support/arg_list.h"
+
+#define LIBC_PRINTF_DEFINE_MODULAR
+#include "src/stdio/printf_core/float_dec_converter.h"
+#include "src/stdio/printf_core/float_hex_converter.h"
+#include "src/stdio/printf_core/parser.h"
+
+namespace LIBC_NAMESPACE_DECL {
+namespace printf_core {
+template class Parser;
+template class Parser>;
+template class Parser>;
+template class Parser>;
+template class Parser>;
+
+#define INSTANTIATE_CONVERT_FN(NAME)
\
+ template int NAME(
\
+ Writer * writer,
\
+ const FormatSection &to_conv);
\
+ template int NAME(
\
+ Writer * writer,
\
+ const FormatSection &to_conv);
\
+ template int NAME(
\
+ Writer * writer,
\
+ const FormatSection &to_conv);
\
+ template int NAME(
\
+ Writer * writer,
\
+ const FormatSection &to_conv)
+
+INSTANTIATE_CONVERT_FN(convert_float_decimal);
+INSTANTIATE_CONVERT_FN(convert_float_dec_exp);
+INSTANTIATE_CONVERT_FN(convert_float_dec_auto);
+INSTANTIATE_CONVERT_FN(convert_float_hex_exp);
+
+} // namespace printf_core
+} // namespace LIBC_NAMESPACE_DECL
+
+// Bring this file into the link if __printf_float is referenced.
+extern "C" void __printf_float() {}
statham-arm wrote:
OK – of course you're right that those workarounds can easily be added later.
As long as you've thought about this and checked it, I'm happy.
(Now I've slept on it, I think I vaguely recall that one rationale for
armlink's choice of behavior is that it makes partial linking change the
semantics less: if you combine a bunch of `.o` files into one big `.o`, then
passing the combined file to a full link step has a very similar effect to
passing the individual files one by one.)
https://github.com/llvm/llvm-project/pull/147426
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [libc] [libc] Modular printf option (float only) (PR #147426)
@@ -0,0 +1,41 @@
+#ifdef LIBC_COPT_PRINTF_MODULAR
+#include "src/__support/arg_list.h"
+
+#define LIBC_PRINTF_DEFINE_MODULAR
+#include "src/stdio/printf_core/float_dec_converter.h"
+#include "src/stdio/printf_core/float_hex_converter.h"
+#include "src/stdio/printf_core/parser.h"
+
+namespace LIBC_NAMESPACE_DECL {
+namespace printf_core {
+template class Parser;
+template class Parser>;
+template class Parser>;
+template class Parser>;
+template class Parser>;
+
+#define INSTANTIATE_CONVERT_FN(NAME)
\
+ template int NAME(
\
+ Writer * writer,
\
+ const FormatSection &to_conv);
\
+ template int NAME(
\
+ Writer * writer,
\
+ const FormatSection &to_conv);
\
+ template int NAME(
\
+ Writer * writer,
\
+ const FormatSection &to_conv);
\
+ template int NAME(
\
+ Writer * writer,
\
+ const FormatSection &to_conv)
+
+INSTANTIATE_CONVERT_FN(convert_float_decimal);
+INSTANTIATE_CONVERT_FN(convert_float_dec_exp);
+INSTANTIATE_CONVERT_FN(convert_float_dec_auto);
+INSTANTIATE_CONVERT_FN(convert_float_hex_exp);
+
+} // namespace printf_core
+} // namespace LIBC_NAMESPACE_DECL
+
+// Bring this file into the link if __printf_float is referenced.
+extern "C" void __printf_float() {}
statham-arm wrote:
> I'd be surprised if other linkers differ; wasn't this usage the whole point
> of allowing undefined weak references?
Certainly this usage _in general_ is the whole point: a weak reference says "I
won't insist that this function be included in the image, but if it has to be
in the image anyway for some other reason, I'd like to call it."
The more specific question here is what counts as "has to be in the image
anyway": do you need a non-weak reference to the _section itself_, or just to
some section in the same object?
The linker in Arm's proprietary toolchain, `armlink`, takes the former
attitude: garbage collection only follows non-weak references, so that if an
object file containing many sections is pulled in to the link, the sections
will then be independently GCed according to whether each one has a chain of
non-weak references from the root. That's why I was concerned.
You _may_ be right that `armlink` is an outlier and no linker this code cares
about will behave the same way. But personally I'd check them all before being
sure!
https://github.com/llvm/llvm-project/pull/147426
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
