[Support] Move Target/CPU Printing out of CommandLine

[lenary]

Summary

This change is rather more invasive than intended. The main intention
here is to make CommandLine.cpp not rely on llvm/Support/Host.h. Right
now, this reliance is only in 3 superficial places:

• Choosing how to expand response files (in two places)
• Printing the default triple and current CPU in --version output.

The built in version system has a method for adding "extra version
printers", commonly used by several tools (such as llc) to report the
registered targets in the built version of LLVM. It was reasonably easy
to move the logic for printing the default triple and current CPU into
a similar function, and register it with any relevant binaries.

The incompatible change here is that now, even if
LLVM_VERSION_PRINTER_SHOW_HOST_TARGET_INFO is defined, most binaries
will no longer print out the default target triple and cpu when provided
with --version, for instance llvm-as and llvm-dis. This breakage is
intended, but the changes in this patch keep printing the default target
and detected in llc and opt as these were remarked as important
binaries in the LLVM install.

The change to expanding response files may also be controversial, but I
believe that these macros should correspond exactly to the host triple
introspection used before.

[lenary]

Sorry to everyone who has been tagged on this proposed change. There is more context on what it's trying to do here: https://discourse.llvm.org/t/targetparser-auto-generation-of-riscv-cpu-definitions/66419/4?u=lenary

[MaskRay]

I am in favor of the spirit of the change and the direction moving target-specific stuff outside LLVMSupport.

There are tools which have default target specific behavior (llc, llvm-ar, etc) and more which don't.

At least for those which don't have default target specific behavior, the default target and host cpu information from many tools' --version is not really useful.

Tools which don't have #include "llvm/Support/Host.h" should not have the new dependency.

% llvm-dwarfdump --version
LLVM (http://llvm.org/):
  LLVM version 16.0.0git
  Optimized build with assertions.
  Default target: x86_64-unknown-linux-gnu
  Host CPU: skylake-avx512

[lenary]

In D137837#3922026, @MaskRay wrote:

I am in favor of the spirit of the change and the direction moving target-specific stuff outside LLVMSupport.

There are tools which have default target specific behavior (llc, llvm-ar, etc) and more which don't.

At least for those which don't have default target specific behavior, the default target and host cpu information from many tools' --version is not really useful.

Tools which don't have #include "llvm/Support/Host.h" should not have the new dependency.

% llvm-dwarfdump --version
LLVM (http://llvm.org/):
  LLVM version 16.0.0git
  Optimized build with assertions.
  Default target: x86_64-unknown-linux-gnu
  Host CPU: skylake-avx512

Broadly, the way I went about choosing whether we should do this was based on two things:

• If the utility initialized all configured targets, or
• If the utility had a -march/-mcpu command-line flag.

I realise this ends up as an approximation, but I think this broadly follows what you're saying - Edited To Add: do let me know if you think those conditions should be changed, or specific cases where this shouldn't apply

e.g., I haven't added this to llvm-as and llvm-dis, because they didn't, and the same applies to llvm-tblgen.

[MaskRay]

Personally I think the CPU info is not useful for all tools.
For most programs I have run which support --version, they don't print target-triple/CPU information. And a lot among them do have target-specific behavior (e.g. Windows/*NIX distinction). So LLVM CommandLine.cpp doing this by default is very strange to me.
I see that you want to retain the existing behavior, though. I want to learn about others opinions. Perhaps ask on Discourse?

I mentioned llvm/Support/Host.h but that was for a middle ground if there turns out to be sufficient support for retaining the information. So I suggested: for the tools which have + #include "llvm/Support/Host.h" in this diff, I think cl::AddExtraVersionPrinter should be dropped.
But perhaps we just don't need to do that :)

[lenary]

In D137837#3922950, @MaskRay wrote:

Personally I think the CPU info is not useful for all tools.
For most programs I have run which support --version, they don't print target-triple/CPU information. And a lot among them do have target-specific behavior (e.g. Windows/*NIX distinction). So LLVM CommandLine.cpp doing this by default is very strange to me.
I see that you want to retain the existing behavior, though. I want to learn about others opinions. Perhaps ask on Discourse?

I mentioned llvm/Support/Host.h but that was for a middle ground if there turns out to be sufficient support for retaining the information. So I suggested: for the tools which have + #include "llvm/Support/Host.h" in this diff, I think cl::AddExtraVersionPrinter should be dropped.
But perhaps we just don't need to do that :)

I have updates to the whole patch sequence, so I will do this latter change, and then ask for further opinions on discourse. :)

[fpetrogalli]

llvm/lib/Support/CommandLine.cpp

2540–2543

There might be tools that rely on having the ExtraPrinter mechanisms available from struct CommandLineCommonOptions. Would it be possible to just pass the extra printer here as a an argument of print(), and leave the ExtraPrinters mechanism inside CommandLineCommonOptions?

2557–2565

I think you have modified the output here. I believe that now the tools will print 2 empty lines right after " with assertions", while before it was printing only one empty line. Maybe it is not important, but I suspect that there might be some tools that parse the output printed by this function. Better to make sure we can guarantee the same output.

[lenary]

llvm/lib/Support/CommandLine.cpp

2540–2543

I will do this, though I think this is abnormal, as it is not LLVM's responsibility to not break downstream-only code.

2557–2565

I'm concerned about this suggestion, but I'm going to do it.

Again, nothing in LLVM Upstream tests the exact format, or expects it to be stable. There is one test, (modified below, but I'm going to revert the change), which checks for the target-detected output, and that's it.

It's also going to be a big issue if someone is checking the version output of a utility that will no longer do target detection after this change (because, this is the main functional change in this patch). Again, if there was testing of this behaviour I'd be happier.

[fpetrogalli]

llvm/lib/Support/CommandLine.cpp

2540–2543

I am happy with this answer, @lenary . It could be that I am wrong. This, together with the extra bit of "it is not LLVM's responsibility to not break downstream-only code", makes me happy with your current solution. I'l leave it up to you to decide what to do.

2557–2565

Well, it is not a bit change to move line 2542 OS << '\n'; after this is statement? :)

(Or am I missing something?)

[lenary]

llvm/lib/Support/CommandLine.cpp

2557–2565

That suggestion still changes the output, it's just the extra newline is now trailing :)

Sorry it took me a while to get back on this, rebasing and addressing comments on the whole sequence means there's quite a slow turnaround on updates to these patches

llvm/tools/llvm-lto/llvm-lto.cpp

49 ↗

(On Diff #477993)

Will undo this irrelevant change when I apply the patch.

[lenary]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

23 ↗

(On Diff #477993)

@MaskRay This is the only place this header has been added, because the libtool tests actually do look for the output of the target detection information.

[fpetrogalli]

LGTM, thank you!

[lenary]

@MaskRay I put a thread on discourse to get more opinions, as I realise I said I would and hadn't yet: https://discourse.llvm.org/t/rfc-moving-target-info-out-of-version-by-default/66921

[fpetrogalli]

In D137837#3960770, @lenary wrote:

@MaskRay I put a thread on discourse to get more opinions, as I realise I said I would and hadn't yet: https://discourse.llvm.org/t/rfc-moving-target-info-out-of-version-by-default/66921

@lenary - are you wanting for an outcome of the thread on discourse before merging this code? Can the proposal on discourse be done as a follow up of this patch?

Francesco

[MaskRay]

I think this patch can drop cl::AddExtraVersionPrinter(sys::printDefaultTargetAndDetectedCPU); from most tools. Only add them case by case when there is consensus there is a significant value.

Many tools get the default target and CPU information by the generic code and it seems that most don't care about the information from the non-flagship tools.

[fpetrogalli]

In D137837#3976344, @MaskRay wrote:

I think this patch can drop cl::AddExtraVersionPrinter(sys::printDefaultTargetAndDetectedCPU); from most tools. Only add them case by case when there is consensus there is a significant value.

@MaskRay / @lenary can we consider this clean up operation a separate piece of work and move on with the patch-set submission needed to get D137838 in?

We all agreed that the work leading at D137838 is a better solution than D137516 because it is breaking the tblgen / libSupport dependency for all targets. I am really thankful that @lenary offered their time to work on this.
The fact that we are getting so little attention from other members is maybe is a sign that we can just move on and merge it without waiting for more consensus? If anything turn out to be wrong, we can always fix up/revert?

Thank you!

Francesco

[lebedev.ri]

In D137837#3984832, @fpetrogalli wrote:

In D137837#3976344, @MaskRay wrote:

I think this patch can drop cl::AddExtraVersionPrinter(sys::printDefaultTargetAndDetectedCPU); from most tools. Only add them case by case when there is consensus there is a significant value.

@MaskRay / @lenary can we consider this clean up operation a separate piece of work and move on with the patch-set submission needed to get D137838 in?

We all agreed that the work leading at D137838 is a better solution than D137516 because it is breaking the tblgen / libSupport dependency for all targets. I am really thankful that @lenary offered their time to work on this.
The fact that we are getting so little attention from other members is maybe is a sign that we can just move on and merge it without waiting for more consensus? If anything turn out to be wrong, we can always fix up/revert?

Thank you!

Francesco

FWIW, llvm-mca / llvm-exegesis / llc / opt need to continue printing the supported CPU's in --version.
Others not sure, any such cleanup should be on case-by-case basis with separate patches for each.

[lenary]

In D137837#3984848, @lebedev.ri wrote:

In D137837#3984832, @fpetrogalli wrote:

In D137837#3976344, @MaskRay wrote:

I think this patch can drop cl::AddExtraVersionPrinter(sys::printDefaultTargetAndDetectedCPU); from most tools. Only add them case by case when there is consensus there is a significant value.

@MaskRay / @lenary can we consider this clean up operation a separate piece of work and move on with the patch-set submission needed to get D137838 in?

We all agreed that the work leading at D137838 is a better solution than D137516 because it is breaking the tblgen / libSupport dependency for all targets. I am really thankful that @lenary offered their time to work on this.
The fact that we are getting so little attention from other members is maybe is a sign that we can just move on and merge it without waiting for more consensus? If anything turn out to be wrong, we can always fix up/revert?

Thank you!

Francesco

FWIW, llvm-mca / llvm-exegesis / llc / opt need to continue printing the supported CPU's in --version.
Others not sure, any such cleanup should be on case-by-case basis with separate patches for each.

None of these tools print a list of CPUs in the output of --version on main, as far as I can see.

All of them except opt print a list of registered targets, and this functionality is done separately, e.g. https://github.com/llvm/llvm-project/blob/ecabba04a35432ad94447d199cf6127d57415456/llvm/tools/llvm-mca/llvm-mca.cpp#L326 which I am not proposing to change in any of the named tools.

To be clear once again, with this change, if a tool does not use cl::AddExtraVersionPrinter(sys::printDefaultTargetAndDetectedCPU);, then the only two lines it will not output are the Default target: <triple> and Host CPU: <cpu> lines from --version. All other output is unaffected, including lists of Registered Targets, and the actual LLVM version.

[lenary]

In D137837#3976344, @MaskRay wrote:

I think this patch can drop cl::AddExtraVersionPrinter(sys::printDefaultTargetAndDetectedCPU); from most tools. Only add them case by case when there is consensus there is a significant value.

Many tools get the default target and CPU information by the generic code and it seems that most don't care about the information from the non-flagship tools.

I have left the info in llc and opt. All other tools no longer have this info, but I hope it is clear how to re-add it if people want that in future.

Note: LLD does not print this, and clang prints the info a different way.

[lebedev.ri]

In D137837#4000960, @lenary wrote:

In D137837#3976344, @MaskRay wrote:

I think this patch can drop cl::AddExtraVersionPrinter(sys::printDefaultTargetAndDetectedCPU); from most tools. Only add them case by case when there is consensus there is a significant value.

Many tools get the default target and CPU information by the generic code and it seems that most don't care about the information from the non-flagship tools.

I have left the info in llc and opt.

Please, like i asked already, don't break llvm-exegesis and llvm-mca.

All other tools no longer have this info, but I hope it is clear how to re-add it if people want that in future.

Note: LLD does not print this, and clang prints the info a different way.

llvm/include/llvm/Support/Host.h

 #define LLVM_SUPPORT_HOST_H
 
 #include <string>
 
 namespace llvm {
 class MallocAllocator;
 class StringRef;
 template <typename ValueTy, typename AllocatorTy> class StringMap;
+class raw_ostream;
 
 namespace sys {
 
   /// getDefaultTargetTriple() - Return the default target triple the compiler
   /// has been configured to produce code for.
   ///
   /// The target triple is a string in the format of:
   ///   CPU_TYPE-VENDOR-OPERATING_SYSTEM
namespace sys {
   /// \param Features - A string mapping feature names to either
   /// true (if enabled) or false (if disabled). This routine makes no guarantees
   /// about exactly which features may appear in this map, except that they are
   /// all valid LLVM feature names.
   ///
   /// \return - True on success.
   bool getHostCPUFeatures(StringMap<bool, MallocAllocator> &Features);
 
+  /// This is a function compatible with cl::AddExtraVersionPrinter, which adds
+  /// info about the current target triple and detected CPU.
+  void printDefaultTargetAndDetectedCPU(raw_ostream &OS);
+
   namespace detail {
   /// Helper functions to extract HostCPUName from /proc/cpuinfo on linux.
   StringRef getHostCPUNameForPowerPC(StringRef ProcCpuinfoContent);
   StringRef getHostCPUNameForARM(StringRef ProcCpuinfoContent);
   StringRef getHostCPUNameForS390x(StringRef ProcCpuinfoContent);
   StringRef getHostCPUNameForRISCV(StringRef ProcCpuinfoContent);
   StringRef getHostCPUNameForSPARC(StringRef ProcCpuinfoContent);
   StringRef getHostCPUNameForBPF();

llvm/lib/MC/TargetRegistry.cpp

void TargetRegistry::printRegisteredTargetsForVersion(raw_ostream &OS) {
   std::vector<std::pair<StringRef, const Target*> > Targets;
   size_t Width = 0;
   for (const auto &T : TargetRegistry::targets()) {
     Targets.push_back(std::make_pair(T.getName(), &T));
     Width = std::max(Width, Targets.back().first.size());
   }
   array_pod_sort(Targets.begin(), Targets.end(), TargetArraySortFn);
 
+  OS << "\n";
   OS << "  Registered Targets:\n";
   for (const auto &Target : Targets) {
     OS << "    " << Target.first;
     OS.indent(Width - Target.first.size())
         << " - " << Target.second->getShortDescription() << '\n';
   }
   if (Targets.empty())
     OS << "    (none)\n";
 }

llvm/lib/Support/CommandLine.cpp

 #include "llvm-c/Support.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLFunctionalExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/Triple.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
-#include "llvm/Support/Host.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstdlib>
Error ExpansionContext::expandResponseFiles(
   // the end of the stream, so seeing that doesn't indicate a bug.
   assert(FileStack.size() > 0 && Argv.size() == FileStack.back().End);
   return Error::success();
 }
 
 bool cl::expandResponseFiles(int Argc, const char *const *Argv,
                              const char *EnvVar, StringSaver &Saver,
                              SmallVectorImpl<const char *> &NewArgv) {
-  auto Tokenize = Triple(sys::getProcessTriple()).isOSWindows()
-                      ? cl::TokenizeWindowsCommandLine
-                      : cl::TokenizeGNUCommandLine;
+#ifdef _WIN32
+  auto Tokenize = cl::TokenizeWindowsCommandLine;
+#else
+  auto Tokenize = cl::TokenizeGNUCommandLine;
+#endif
   // The environment variable specifies initial options.
   if (EnvVar)
     if (std::optional<std::string> EnvValue = sys::Process::GetEnv(EnvVar))
       Tokenize(*EnvValue, Saver, NewArgv, /*MarkEOLs=*/false);
 
   // Command line options can override the environment variable.
   NewArgv.append(Argv + 1, Argv + Argc);
   ExpansionContext ECtx(Saver.getAllocator(), Tokenize);
bool CommandLineParser::ParseCommandLineOptions(int argc,
   bool IgnoreErrors = Errs;
   if (!Errs)
     Errs = &errs();
   bool ErrorParsing = false;
 
   // Expand response files.
   SmallVector<const char *, 20> newArgv(argv, argv + argc);
   BumpPtrAllocator A;
-  ExpansionContext ECtx(A, Triple(sys::getProcessTriple()).isOSWindows()
-                               ? cl::TokenizeWindowsCommandLine
-                               : cl::TokenizeGNUCommandLine);
+#ifdef _WIN32
+  auto Tokenize = cl::TokenizeWindowsCommandLine;
+#else
+  auto Tokenize = cl::TokenizeGNUCommandLine;
+#endif
+  ExpansionContext ECtx(A, Tokenize);
   if (Error Err = ECtx.expandResponseFiles(newArgv)) {
     *Errs << toString(std::move(Err)) << '\n';
     return false;
   }
   argv = &newArgv[0];
   argc = static_cast<int>(newArgv.size());
 
   // Copy the program name into ProgName, making sure not to overflow it.

 # endif
 #else
 // Otherwise, for an unknown compiler, assume this is an optimized build.
 # define LLVM_IS_DEBUG_BUILD 0
 #endif
 
 namespace {
 class VersionPrinter {
 public:
-  void print() {
+  void print(std::vector<VersionPrinterTy> ExtraPrinters = {}) {
     raw_ostream &OS = outs();
 #ifdef PACKAGE_VENDOR
     OS << PACKAGE_VENDOR << " ";
 #else
     OS << "LLVM (http://llvm.org/):\n  ";
 #endif
     OS << PACKAGE_NAME << " version " << PACKAGE_VERSION << "\n  ";
 #if LLVM_IS_DEBUG_BUILD
     OS << "DEBUG build";
 #else
     OS << "Optimized build";
 #endif
 #ifndef NDEBUG
     OS << " with assertions";
 #endif
-#if LLVM_VERSION_PRINTER_SHOW_HOST_TARGET_INFO
-    std::string CPU = std::string(sys::getHostCPUName());
-    if (CPU == "generic")
-      CPU = "(unknown)";
-    OS << ".\n"
-       << "  Default target: " << sys::getDefaultTargetTriple() << '\n'
-       << "  Host CPU: " << CPU;
-#endif
-    OS << '\n';
+    OS << ".\n";
+
+    // Iterate over any registered extra printers and call them to add further
+    // information.
+    if (!ExtraPrinters.empty()) {
+      for (const auto &I : ExtraPrinters)
+        I(outs());
+    }
   }
   void operator=(bool OptionWasSpecified);
 };
 
 struct CommandLineCommonOptions {
   // Declare the four HelpPrinter instances that are used to print out help, or
   // help-hidden as an uncategorized list or in categories.
   HelpPrinter UncategorizedNormalPrinter{false};
   HelpPrinter UncategorizedHiddenPrinter{true};

 void VersionPrinter::operator=(bool OptionWasSpecified) {
   if (!OptionWasSpecified)
     return;
 
   if (CommonOptions->OverrideVersionPrinter != nullptr) {
     CommonOptions->OverrideVersionPrinter(outs());
     exit(0);
   }
-  print();
-
-  // Iterate over any registered extra printers and call them to add further
-  // information.
-  if (!CommonOptions->ExtraVersionPrinters.empty()) {
-    outs() << '\n';
-    for (const auto &I : CommonOptions->ExtraVersionPrinters)
-      I(outs());
-  }
+  print(CommonOptions->ExtraVersionPrinters);
 
   exit(0);
 }
 
 void HelpPrinterWrapper::operator=(bool Value) {
   if (!Value)
     return;
 
void cl::PrintHelpMessage(bool Hidden, bool Categorized) {
   else if (Hidden && !Categorized)
     CommonOptions->UncategorizedHiddenPrinter.printHelp();
   else
     CommonOptions->CategorizedHiddenPrinter.printHelp();
 }
 
 /// Utility function for printing version number.
 void cl::PrintVersionMessage() {
-  CommonOptions->VersionPrinterInstance.print();
+  CommonOptions->VersionPrinterInstance.print(CommonOptions->ExtraVersionPrinters);
 }
 
 void cl::SetVersionPrinter(VersionPrinterTy func) {
   CommonOptions->OverrideVersionPrinter = func;
 }
 
 void cl::AddExtraVersionPrinter(VersionPrinterTy func) {
   CommonOptions->ExtraVersionPrinters.push_back(func);

llvm/lib/Support/Host.cpp

std::string sys::getProcessTriple() {
 
   if (sizeof(void *) == 8 && PT.isArch32Bit())
     PT = PT.get64BitArchVariant();
   if (sizeof(void *) == 4 && PT.isArch64Bit())
     PT = PT.get32BitArchVariant();
 
   return PT.str();
 }
+
+void sys::printDefaultTargetAndDetectedCPU(raw_ostream &OS) {
+#if LLVM_VERSION_PRINTER_SHOW_HOST_TARGET_INFO
+  std::string CPU = std::string(sys::getHostCPUName());
+  if (CPU == "generic")
+    CPU = "(unknown)";
+  OS << "  Default target: " << sys::getDefaultTargetTriple() << '\n'
+     << "  Host CPU: " << CPU << '\n';
+#endif
+}

llvm/test/tools/llvm-libtool-darwin/ignored-options.test

 ## This tests some flags are correctly parsed even when they're not handled
 
 # RUN: llvm-libtool-darwin -V -syslibroot foo | FileCheck %s
 # RUN: llvm-libtool-darwin -h | FileCheck --check-prefix=HELP %s
 
-# CHECK: Default target:
+# CHECK: LLVM version
 
 # HELP-NOT: syslibroot

llvm/tools/llc/llc.cpp

int main(int argc, char **argv) {
   initializeHardwareLoopsPass(*Registry);
   initializeTransformUtils(*Registry);
   initializeReplaceWithVeclibLegacyPass(*Registry);
   initializeTLSVariableHoistLegacyPassPass(*Registry);
 
   // Initialize debugging passes.
   initializeScavengerTestPass(*Registry);
 
+  // Register the Target and CPU printer for --version.
+  cl::AddExtraVersionPrinter(sys::printDefaultTargetAndDetectedCPU);
   // Register the target printer for --version.
   cl::AddExtraVersionPrinter(TargetRegistry::printRegisteredTargetsForVersion);
 
   cl::ParseCommandLineOptions(argc, argv, "llvm system compiler\n");
 
   if (TimeTrace)
     timeTraceProfilerInitialize(TimeTraceGranularity, argv[0]);
   auto TimeTraceScopeExit = make_scope_exit([]() {

llvm/tools/opt/opt.cpp

PassPlugins.setCallback([&](const std::string &PluginPath) {
     if (!Plugin) {
       errs() << "Failed to load passes from '" << PluginPath
              << "'. Request ignored.\n";
       return;
     }
     PluginList.emplace_back(Plugin.get());
   });
 
+  // Register the Target and CPU printer for --version.
+  cl::AddExtraVersionPrinter(sys::printDefaultTargetAndDetectedCPU);
+
   cl::ParseCommandLineOptions(argc, argv,
     "llvm .bc -> .bc modular optimizer and analysis printer\n");
 
   LLVMContext Context;
 
   // If `-passes=` is specified, use NPM.
   // If `-enable-new-pm` is specified and there are no codegen passes, use NPM.
   // e.g. `-enable-new-pm -sroa` will use NPM.