Triple: Forward declare Twine and remove include #145685
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-mc @llvm/pr-subscribers-clang Author: Matt Arsenault (arsenm) ChangesIt turns out real Twine usage is scarce, and seems to only Full diff: https://github.com/llvm/llvm-project/pull/145685.diff 14 Files Affected:
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp
index 709cf3bb87c8e..614e8aec3db24 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1464,8 +1464,7 @@ static IdentifierInfo *ExpectFeatureIdentifierInfo(Token &Tok,
/// Implements the __is_target_arch builtin macro.
static bool isTargetArch(const TargetInfo &TI, const IdentifierInfo *II) {
- std::string ArchName = II->getName().lower() + "--";
- llvm::Triple Arch(ArchName);
+ llvm::Triple Arch(II->getName().lower() + "--");
const llvm::Triple &TT = TI.getTriple();
if (TT.isThumb()) {
// arm matches thumb or thumbv7. armv7 matches thumbv7.
@@ -1494,9 +1493,7 @@ static bool isTargetVendor(const TargetInfo &TI, const IdentifierInfo *II) {
/// Implements the __is_target_os builtin macro.
static bool isTargetOS(const TargetInfo &TI, const IdentifierInfo *II) {
- std::string OSName =
- (llvm::Twine("unknown-unknown-") + II->getName().lower()).str();
- llvm::Triple OS(OSName);
+ llvm::Triple OS(llvm::Twine("unknown-unknown-") + II->getName().lower());
if (OS.getOS() == llvm::Triple::Darwin) {
// Darwin matches macos, ios, etc.
return TI.getTriple().isOSDarwin();
@@ -1507,12 +1504,11 @@ static bool isTargetOS(const TargetInfo &TI, const IdentifierInfo *II) {
/// Implements the __is_target_environment builtin macro.
static bool isTargetEnvironment(const TargetInfo &TI,
const IdentifierInfo *II) {
- std::string EnvName = (llvm::Twine("---") + II->getName().lower()).str();
- llvm::Triple Env(EnvName);
+ llvm::Triple Env(llvm::Twine("---") + II->getName().lower());
// The unknown environment is matched only if
// '__is_target_environment(unknown)' is used.
if (Env.getEnvironment() == llvm::Triple::UnknownEnvironment &&
- EnvName != "---unknown")
+ Env.getEnvironmentName() != "unknown")
return false;
return TI.getTriple().getEnvironment() == Env.getEnvironment();
}
@@ -1524,9 +1520,7 @@ static bool isTargetVariantOS(const TargetInfo &TI, const IdentifierInfo *II) {
if (!VariantTriple)
return false;
- std::string OSName =
- (llvm::Twine("unknown-unknown-") + II->getName().lower()).str();
- llvm::Triple OS(OSName);
+ llvm::Triple OS(llvm::Twine("unknown-unknown-") + II->getName().lower());
if (OS.getOS() == llvm::Triple::Darwin) {
// Darwin matches macos, ios, etc.
return VariantTriple->isOSDarwin();
@@ -1543,8 +1537,7 @@ static bool isTargetVariantEnvironment(const TargetInfo &TI,
const llvm::Triple *VariantTriple = TI.getDarwinTargetVariantTriple();
if (!VariantTriple)
return false;
- std::string EnvName = (llvm::Twine("---") + II->getName().lower()).str();
- llvm::Triple Env(EnvName);
+ llvm::Triple Env(llvm::Twine("---") + II->getName().lower());
return VariantTriple->getEnvironment() == Env.getEnvironment();
}
return false;
diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h
index 1865be6e95dea..96ee993a57fbc 100644
--- a/llvm/include/llvm/TargetParser/Triple.h
+++ b/llvm/include/llvm/TargetParser/Triple.h
@@ -9,7 +9,7 @@
#ifndef LLVM_TARGETPARSER_TRIPLE_H
#define LLVM_TARGETPARSER_TRIPLE_H
-#include "llvm/ADT/Twine.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/VersionTuple.h"
@@ -20,6 +20,7 @@
#undef sparc
namespace llvm {
+class Twine;
/// Triple - Helper class for working with autoconf configuration names. For
/// historical reasons, we also call these 'triples' (they used to contain
@@ -349,7 +350,12 @@ class Triple {
/// triple fields unknown.
Triple() = default;
+ LLVM_ABI explicit Triple(std::string &&Str);
+ LLVM_ABI explicit Triple(StringRef Str) : Triple(Str.str()) {}
LLVM_ABI explicit Triple(const Twine &Str);
+ LLVM_ABI explicit Triple(const char *Str) : Triple(std::string(Str)) {}
+ LLVM_ABI explicit Triple(const std::string &Str) : Triple(std::string(Str)) {}
+
LLVM_ABI Triple(const Twine &ArchStr, const Twine &VendorStr,
const Twine &OSStr);
LLVM_ABI Triple(const Twine &ArchStr, const Twine &VendorStr,
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index 926dc6211eb8d..bc9a0a079f7a2 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -633,7 +633,7 @@ bool LLParser::parseTargetDefinition(std::string &TentativeDLStr,
if (parseToken(lltok::equal, "expected '=' after target triple") ||
parseStringConstant(Str))
return true;
- M->setTargetTriple(Triple(Str));
+ M->setTargetTriple(Triple(std::move(Str)));
return false;
case lltok::kw_datalayout:
Lex.Lex();
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index fde934fbb3cf1..3d28e27bb2b9e 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -4695,7 +4695,7 @@ Error BitcodeReader::parseModule(uint64_t ResumeBit,
std::string S;
if (convertToString(Record, 0, S))
return error("Invalid record");
- TheModule->setTargetTriple(Triple(S));
+ TheModule->setTargetTriple(Triple(std::move(S)));
break;
}
case bitc::MODULE_CODE_DATALAYOUT: { // DATALAYOUT: [strchr x N]
diff --git a/llvm/lib/CodeGen/CodeGenTargetMachineImpl.cpp b/llvm/lib/CodeGen/CodeGenTargetMachineImpl.cpp
index 4a3503a2da7db..125ffd597bf23 100644
--- a/llvm/lib/CodeGen/CodeGenTargetMachineImpl.cpp
+++ b/llvm/lib/CodeGen/CodeGenTargetMachineImpl.cpp
@@ -197,7 +197,7 @@ CodeGenTargetMachineImpl::createMCStreamer(raw_pwrite_stream &Out,
return make_error<StringError>("createMCAsmBackend failed",
inconvertibleErrorCode());
- Triple T(getTargetTriple().str());
+ Triple T(getTargetTriple());
AsmStreamer.reset(getTarget().createMCObjectStreamer(
T, Context, std::unique_ptr<MCAsmBackend>(MAB),
DwoOut ? MAB->createDwoObjectWriter(Out, *DwoOut)
diff --git a/llvm/lib/MC/MCDisassembler/Disassembler.h b/llvm/lib/MC/MCDisassembler/Disassembler.h
index 3cb2479d388f6..4c34d86437b1f 100644
--- a/llvm/lib/MC/MCDisassembler/Disassembler.h
+++ b/llvm/lib/MC/MCDisassembler/Disassembler.h
@@ -98,7 +98,7 @@ class LLVMDisasmContext {
MAI(std::move(MAI)), MRI(std::move(MRI)), MSI(std::move(MSI)),
MII(std::move(MII)), Ctx(std::move(Ctx)), DisAsm(std::move(DisAsm)),
IP(std::move(IP)), Options(0), CommentStream(CommentsToEmit) {}
- const std::string &getTripleName() const { return TripleName; }
+ StringRef getTripleName() const { return TripleName; }
void *getDisInfo() const { return DisInfo; }
int getTagType() const { return TagType; }
LLVMOpInfoCallback getGetOpInfo() const { return GetOpInfo; }
diff --git a/llvm/lib/MC/MCSectionELF.cpp b/llvm/lib/MC/MCSectionELF.cpp
index 72a959b1c9208..181c4ff25d541 100644
--- a/llvm/lib/MC/MCSectionELF.cpp
+++ b/llvm/lib/MC/MCSectionELF.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/MC/MCSectionELF.h"
+#include "llvm/ADT/Twine.h"
#include "llvm/BinaryFormat/ELF.h"
#include "llvm/MC/MCAsmInfo.h"
#include "llvm/MC/MCExpr.h"
diff --git a/llvm/lib/MC/MCSubtargetInfo.cpp b/llvm/lib/MC/MCSubtargetInfo.cpp
index d86eaad48420d..89a08327dd259 100644
--- a/llvm/lib/MC/MCSubtargetInfo.cpp
+++ b/llvm/lib/MC/MCSubtargetInfo.cpp
@@ -9,6 +9,7 @@
#include "llvm/MC/MCSubtargetInfo.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
#include "llvm/MC/MCInstrItineraries.h"
#include "llvm/MC/MCSchedule.h"
#include "llvm/Support/Format.h"
diff --git a/llvm/lib/Object/ArchiveWriter.cpp b/llvm/lib/Object/ArchiveWriter.cpp
index c61ba868efe60..6fc0889afc6a8 100644
--- a/llvm/lib/Object/ArchiveWriter.cpp
+++ b/llvm/lib/Object/ArchiveWriter.cpp
@@ -700,7 +700,7 @@ static bool isECObject(object::SymbolicFile &Obj) {
getBitcodeTargetTriple(Obj.getMemoryBufferRef());
if (!TripleStr)
return false;
- Triple T(*TripleStr);
+ Triple T(std::move(*TripleStr));
return T.isWindowsArm64EC() || T.getArch() == Triple::x86_64;
}
@@ -719,7 +719,7 @@ static bool isAnyArm64COFF(object::SymbolicFile &Obj) {
getBitcodeTargetTriple(Obj.getMemoryBufferRef());
if (!TripleStr)
return false;
- Triple T(*TripleStr);
+ Triple T(std::move(*TripleStr));
return T.isOSWindows() && T.getArch() == Triple::aarch64;
}
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h b/llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
index 23826cc22250a..7c24f428d78e4 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
+++ b/llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h
@@ -14,6 +14,7 @@
#ifndef LLVM_LIB_TARGET_AMDGPU_AMDGPUSUBTARGET_H
#define LLVM_LIB_TARGET_AMDGPU_AMDGPUSUBTARGET_H
+#include "llvm/ADT/SmallVector.h"
#include "llvm/IR/CallingConv.h"
#include "llvm/Support/Alignment.h"
#include "llvm/TargetParser/Triple.h"
diff --git a/llvm/lib/TargetParser/CSKYTargetParser.cpp b/llvm/lib/TargetParser/CSKYTargetParser.cpp
index 006d2bb342acc..5fea08e38e2db 100644
--- a/llvm/lib/TargetParser/CSKYTargetParser.cpp
+++ b/llvm/lib/TargetParser/CSKYTargetParser.cpp
@@ -13,6 +13,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/TargetParser/CSKYTargetParser.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringSwitch.h"
using namespace llvm;
diff --git a/llvm/lib/TargetParser/LoongArchTargetParser.cpp b/llvm/lib/TargetParser/LoongArchTargetParser.cpp
index db68498609b57..572a278a39fce 100644
--- a/llvm/lib/TargetParser/LoongArchTargetParser.cpp
+++ b/llvm/lib/TargetParser/LoongArchTargetParser.cpp
@@ -12,6 +12,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/TargetParser/LoongArchTargetParser.h"
+#include "llvm/ADT/SmallVector.h"
using namespace llvm;
using namespace llvm::LoongArch;
diff --git a/llvm/lib/TargetParser/Triple.cpp b/llvm/lib/TargetParser/Triple.cpp
index 1fc22295a0ce2..cf7c5cdb070ad 100644
--- a/llvm/lib/TargetParser/Triple.cpp
+++ b/llvm/lib/TargetParser/Triple.cpp
@@ -1018,7 +1018,7 @@ static Triple::ObjectFormatType getDefaultFormat(const Triple &T) {
///
/// This stores the string representation and parses the various pieces into
/// enum members.
-Triple::Triple(const Twine &Str) : Data(Str.str()) {
+Triple::Triple(std::string &&Str) : Data(std::move(Str)) {
// Do minimal parsing by hand here.
SmallVector<StringRef, 4> Components;
StringRef(Data).split(Components, '-', /*MaxSplit*/ 3);
@@ -1049,6 +1049,8 @@ Triple::Triple(const Twine &Str) : Data(Str.str()) {
ObjectFormat = getDefaultFormat(*this);
}
+Triple::Triple(const Twine &Str) : Triple(Str.str()) {}
+
/// Construct a triple from string representations of the architecture,
/// vendor, and OS.
///
diff --git a/llvm/lib/TargetParser/Unix/Host.inc b/llvm/lib/TargetParser/Unix/Host.inc
index a33fe6fff8936..ef9e288ee3a01 100644
--- a/llvm/lib/TargetParser/Unix/Host.inc
+++ b/llvm/lib/TargetParser/Unix/Host.inc
@@ -55,7 +55,7 @@ static std::string updateTripleOSVersion(std::string TargetTripleString) {
// On AIX, the AIX version and release should be that of the current host
// unless if the version has already been specified.
if (Triple(LLVM_HOST_TRIPLE).getOS() == Triple::AIX) {
- Triple TT(TargetTripleString);
+ Triple TT(std::move(TargetTripleString));
if (TT.getOS() == Triple::AIX && !TT.getOSMajorVersion()) {
struct utsname name;
if (uname(&name) != -1) {
|
It turns out real Twine usage is scarce, and seems to only be used from clang. Add a few overloads for the common cases, and introduce a string&& case as the base case.
compnerd
reviewed
Jun 25, 2025
| LLVM_ABI explicit Triple(StringRef Str) : Triple(Str.str()) {} | ||
| LLVM_ABI explicit Triple(const Twine &Str); | ||
| LLVM_ABI explicit Triple(const char *Str) : Triple(std::string(Str)) {} | ||
| LLVM_ABI explicit Triple(const std::string &Str) : Triple(std::string(Str)) {} |
Member
There was a problem hiding this comment.
I'm not sure if we should mark the inline ctors as ABI - do we want to emit a canonical version of them? It seems like those should all get folded alway through inlining into the out of line Triple(std::string &&) call.
Contributor
There was a problem hiding this comment.
Yes, @compnerd is right. Please leave-off the LLVM_ABI annotation from any method defined entirely in the header.
We should eventually be able to flag this via a static analysis CI job.
nikic
reviewed
Jun 25, 2025
| /// triple fields unknown. | ||
| Triple() = default; | ||
|
|
||
| LLVM_ABI explicit Triple(std::string &&Str); |
Contributor
There was a problem hiding this comment.
Why std::string && and const std::string & rather than one std::string ctor?
Contributor
Author
There was a problem hiding this comment.
Avoiding string copies, most of the uses (particularly the local uses) are just producing temporary strings that can be claimed
Contributor
There was a problem hiding this comment.
Probably my C++-foo is not strong enough, but I thought that doing a by-value pass + std::move gives you the same behavior (i.e. temporary or std::move argument doesn't get copied, everything else does).
Contributor
There was a problem hiding this comment.
That is indeed the case. Technically there is an additional move, OTOH that pretty much never matters, since the optimizer is almost always perfectly capable of removing that.
arsenm
force-pushed
the
users/arsenm/triple/forward-declare-twine
branch
from
a0b41a8 to
57db18c
Compare
MaskRay
reviewed
Jun 26, 2025
| LLVM_ABI explicit Triple(std::string &&Str); | ||
| explicit Triple(StringRef Str) : Triple(Str.str()) {} | ||
| explicit Triple(const char *Str) : Triple(std::string(Str)) {} | ||
| explicit Triple(const std::string &Str) : Triple(std::string(Str)) {} |
Member
There was a problem hiding this comment.
With the StringRef overload (consider dropping explicit?), is Triple(const std::string &Str) still useful?
Contributor
Author
There was a problem hiding this comment.
This avoids ambiguous conversion to Twine or StringRef
MaskRay
approved these changes
Jun 26, 2025
anthonyhatran
pushed a commit
to anthonyhatran/llvm-project
that referenced
this pull request
Jun 26, 2025
Collaborator
|
This commit has been identified as causing a change in Clang driver behaviour. The observed symptom is that the Clang driver is passing |
Contributor
|
@hubert-reinterpretcast @arsenm I have raised a follow-up PR for fixing the issue on AIX. |
tonykuttai
added a commit
that referenced
this pull request
Jul 8, 2025
…arget triple on AIX (#147488) PR #145685 introduced constructor overload ambiguity in the Triple class, causing `updateTripleOSVersion()` to construct Triple objects with `unknown` instead of the configured target triple (e.g., `powerpc-ibm-aix7.3.0.0`). This results in Clang driver errors like `error: unknown target triple 'unknown'`. Used `Twine` constructor with braced initialization to bypass ambiguity. --------- Co-authored-by: Tony Varghese <[email protected]> Co-authored-by: Matt Arsenault <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It turns out real Twine usage is scarce, and seems to only
be used from clang. Add a few overloads for the common cases,
and introduce a string&& case as the base case.