Page MenuHomePhabricator

[Support] Move Target/CPU Printing out of CommandLine
ClosedPublic

Authored by lenary on Nov 11 2022, 6:39 AM.

Details

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.

Diff Detail

Event Timeline

lenary created this revision.Nov 11 2022, 6:39 AM
lenary requested review of this revision.Nov 11 2022, 6:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2022, 6:39 AM

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 added a comment.EditedNov 11 2022, 11:12 AM

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 added a comment.EditedNov 11 2022, 11:50 AM

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 added a comment.EditedNov 11 2022, 11:09 PM

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

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 added inline comments.
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 added inline comments.Nov 25 2022, 8:58 AM
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 added inline comments.Nov 25 2022, 9:09 AM
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 updated this revision to Diff 477993.Nov 25 2022, 9:38 AM
lenary edited the summary of this revision. (Show Details)
lenary added inline comments.Nov 25 2022, 9:41 AM
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 added inline comments.Nov 25 2022, 9:45 AM
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 accepted this revision.Nov 28 2022, 2:11 AM

LGTM, thank you!

This revision is now accepted and ready to land.Nov 28 2022, 2:11 AM

@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

@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 added a comment.EditedDec 6 2022, 4:06 PM

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

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.

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 updated this revision to Diff 483486.Dec 16 2022, 4:04 AM
lenary edited the summary of this revision. (Show Details)
lenary added a comment.EditedDec 16 2022, 4:07 AM

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.

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.

lenary updated this revision to Diff 483510.Dec 16 2022, 6:17 AM
MaskRay accepted this revision.Dec 16 2022, 2:42 PM
This revision was landed with ongoing or failed builds.Dec 20 2022, 1:56 AM
This revision was automatically updated to reflect the committed changes.