Triple: Forward declare Twine and remove include #145685

arsenm · 2025-06-25T12:04:47Z

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.

arsenm · 2025-06-25T12:05:06Z

Triple: Forward declare Twine and remove include #145685 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

llvmbot · 2025-06-25T12:07:22Z

@llvm/pr-subscribers-mc
@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-clang

Author: Matt Arsenault (arsenm)

Changes

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.

Full diff: https://github.com/llvm/llvm-project/pull/145685.diff

14 Files Affected:

(modified) clang/lib/Lex/PPMacroExpansion.cpp (+6-13)
(modified) llvm/include/llvm/TargetParser/Triple.h (+7-1)
(modified) llvm/lib/AsmParser/LLParser.cpp (+1-1)
(modified) llvm/lib/Bitcode/Reader/BitcodeReader.cpp (+1-1)
(modified) llvm/lib/CodeGen/CodeGenTargetMachineImpl.cpp (+1-1)
(modified) llvm/lib/MC/MCDisassembler/Disassembler.h (+1-1)
(modified) llvm/lib/MC/MCSectionELF.cpp (+1)
(modified) llvm/lib/MC/MCSubtargetInfo.cpp (+1)
(modified) llvm/lib/Object/ArchiveWriter.cpp (+2-2)
(modified) llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h (+1)
(modified) llvm/lib/TargetParser/CSKYTargetParser.cpp (+1)
(modified) llvm/lib/TargetParser/LoongArchTargetParser.cpp (+1)
(modified) llvm/lib/TargetParser/Triple.cpp (+3-1)
(modified) llvm/lib/TargetParser/Unix/Host.inc (+1-1)

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.

lenary

LGTM

compnerd

CC: @andrurogerz

compnerd · 2025-06-25T18:26:11Z

llvm/include/llvm/TargetParser/Triple.h

+  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)) {}


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.

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 · 2025-06-25T18:37:45Z

llvm/include/llvm/TargetParser/Triple.h

  /// triple fields unknown.
  Triple() = default;

+  LLVM_ABI explicit Triple(std::string &&Str);


Why std::string && and const std::string & rather than one std::string ctor?

Avoiding string copies, most of the uses (particularly the local uses) are just producing temporary strings that can be claimed

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).

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.

MaskRay · 2025-06-26T04:45:54Z

llvm/include/llvm/TargetParser/Triple.h

+  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)) {}


With the StringRef overload (consider dropping explicit?), is Triple(const std::string &Str) still useful?

This avoids ambiguous conversion to Twine or StringRef

hubert-reinterpretcast · 2025-07-07T15:35:50Z

This commit has been identified as causing a change in Clang driver behaviour. The observed symptom is that the Clang driver is passing unknown to -cc1 instead of the configured default target triple of powerpc-ibm-aix7.3.0.0. Investigation is ongoing but we may need to revert this due to major impacts to our internal environment.

tonykuttai · 2025-07-08T09:10:02Z

@hubert-reinterpretcast @arsenm I have raised a follow-up PR for fixing the issue on AIX.
[PowerPC][clang] Fix triple constructor ambiguity causing "unknown" target triple on AIX

…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]>

arsenm requested review from MaskRay and nikic June 25, 2025 12:06

arsenm marked this pull request as ready for review June 25, 2025 12:06

llvmbot added clang Clang issues not falling into any other category backend:AMDGPU clang:frontend Language frontend issues, e.g. anything involving "Sema" llvm:codegen llvm:mc Machine (object) code backend:CSKY llvm:binary-utilities labels Jun 25, 2025

Triple: Forward declare Twine and remove include

504e294

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.

lenary approved these changes Jun 25, 2025

View reviewed changes

compnerd reviewed Jun 25, 2025

View reviewed changes

nikic reviewed Jun 25, 2025

View reviewed changes

Remove LLVM_ABI from inline constructors

57db18c

arsenm force-pushed the users/arsenm/triple/forward-declare-twine branch from a0b41a8 to 57db18c Compare June 25, 2025 22:44

MaskRay reviewed Jun 26, 2025

View reviewed changes

MaskRay approved these changes Jun 26, 2025

View reviewed changes

arsenm merged commit fff720d into main Jun 26, 2025
7 checks passed

arsenm deleted the users/arsenm/triple/forward-declare-twine branch June 26, 2025 06:26

anthonyhatran pushed a commit to anthonyhatran/llvm-project that referenced this pull request Jun 26, 2025

Triple: Forward declare Twine and remove include (llvm#145685)

fa3d604

tonykuttai mentioned this pull request Jul 8, 2025

[PowerPC][clang] Fix triple constructor ambiguity causing "unknown" target triple on AIX #147488

Merged

Triple: Forward declare Twine and remove include #145685

Triple: Forward declare Twine and remove include #145685

Uh oh!

Conversation

arsenm commented Jun 25, 2025

Uh oh!

arsenm commented Jun 25, 2025

Uh oh!

llvmbot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lenary left a comment

Choose a reason for hiding this comment

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

compnerd Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

andrurogerz Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

nikic Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

arsenm Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

nikic Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

philnik777 Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

MaskRay Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arsenm Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hubert-reinterpretcast commented Jul 7, 2025

Uh oh!

tonykuttai commented Jul 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

llvmbot commented Jun 25, 2025 •

edited

Loading

MaskRay Jun 26, 2025 •

edited

Loading