This is an archive of the discontinued LLVM Phabricator instance.

[Support] Move TargetParsers to new component
ClosedPublic

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

Details

Summary

This is a fairly large changeset, but it can be broken into a few
pieces:

  • llvm/Support/*TargetParser* are all moved from the LLVM Support component into a new LLVM Component called "TargetParser". This potentially enables using tablegen to maintain this information, as is shown in https://reviews.llvm.org/D137517. This cannot currently be done, as llvm-tblgen relies on LLVM's Support component.
  • This also moves two files from Support which use and depend on information in the TargetParser:
    • llvm/Support/Host.{h,cpp} which contains functions for inspecting the current Host machine for info about it, primarily to support getting the host triple, but also for -mcpu=native support in e.g. Clang. This is fairly tightly intertwined with the information in X86TargetParser.h, so keeping them in the same component makes sense.
    • llvm/ADT/Triple.h and llvm/Support/Triple.cpp, which contains the target triple parser and representation. This is very intertwined with the Arm target parser, because the arm architecture version appears in canonical triples on arm platforms.
  • I moved the relevant unittests to their own directory.

And so, we end up with a single component that has all the information
about the following, which to me seems like a unified component:

  • Triples that LLVM Knows about
  • Architecture names and CPUs that LLVM knows about
  • CPU detection logic for LLVM

Given this, I have also moved RISCVISAInfo.h into this component, as
it seems to me to be part of that same set of functionality.

If you get link errors in your components after this patch, you likely
need to add TargetParser into LLVM_LINK_COMPONENTS in CMake.

Diff Detail

Event Timeline

lenary created this revision.Nov 11 2022, 6:40 AM
Herald added a reviewer: andreadb. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a reviewer: rafauler. · View Herald Transcript
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a reviewer: ributzka. · View Herald Transcript
Herald added a reviewer: njames93. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
lenary requested review of this revision.Nov 11 2022, 6:40 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: zuban32. · View Herald Transcript
Herald added a reviewer: dang. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript

Apologies to everyone who has automatically been marked as a reviewer for this change. There is more context for it here: https://discourse.llvm.org/t/targetparser-auto-generation-of-riscv-cpu-definitions/66419/4?u=lenary

thakis added a subscriber: thakis.Nov 11 2022, 9:14 AM

This is a gigantic diff. I'd recommend keeping the .h files in the old place for v0 and make them just forwarding headers that include the header at the new location. That way, you don't have to update oodles of include lines in this patch, and it makes it a bit easier to see what's going on. (You can then update all the include lines in a trivial follow-up if this change goes through, and then remove the forwarding headers in Support, to cut the dependency you want to remove.)

This is a gigantic diff. I'd recommend keeping the .h files in the old place for v0 and make them just forwarding headers that include the header at the new location. That way, you don't have to update oodles of include lines in this patch, and it makes it a bit easier to see what's going on. (You can then update all the include lines in a trivial follow-up if this change goes through, and then remove the forwarding headers in Support, to cut the dependency you want to remove.)

Thanks for the suggestion. I do agree this patch is too big to land as-is. I think this patch is useful as "this shows the full effect of this change", even if we find ways to make how we land this patch less invasive. One other thought I had was to move the unittests first, but that doesn't make as big a difference as the fact that llvm/ADT/Triple.h is referenced everywhere.

I would like more comments (either here or on Discourse, link in a prior comment) on whether this split is reasonable before I post a v2 of this patch, though.

Hi @lenary - thank you for working on this!

The patch is reasonable to me.

I agree on the suggestion of using forwarding headers or the first iteration of the change, it will make it easier to review.

I'll adapt the auto-get patch at D137517 on D137838.

As for the name TargetParser. @arsenm came up with the name SubtargetRegistry. I am fine with either names.

Francesco

Moving target-specific parsers outside LLVMSupport LGTM. I even objected a bit when more stuff of this kind was introduced into LLVMSupport.

+1 for adding temporary forwarding headers for now to avoid update all #include users.

lenary updated this revision to Diff 475863.Nov 16 2022, 10:14 AM
lenary edited the summary of this revision. (Show Details)

I've updated the patch to use forwarding headers (and to rebase past some changes that have happened in the interim).

The patch is still huge because of the number of places using the TargetParsers, which need the component added to their CMakeLists.txt.

I think after the next release, I will add #warning to the forwarding headers, so their usage can be noticed and updated.

@lenary - thank you for the update!

I have added a bunch of miso comments, a bit repetitive... by the time I realised they were repetitive it was faster to get to the bottom of it than removing them!

I've updated the patch to use forwarding headers (and to rebase past some changes that have happened in the interim).

The patch is still huge because of the number of places using the TargetParsers, which need the component added to their CMakeLists.txt.

I think after the next release, I will add #warning to the forwarding headers, so their usage can be noticed and updated.

Why not add the #warning directive now?

Thank you again,

Francesco

clang/lib/Lex/CMakeLists.txt
3–6

I wonder how support was not being detected as a linking error...

clang/lib/Tooling/DumpTool/CMakeLists.txt
5 ↗(On Diff #475863)

nit: unrelated change.

flang/tools/bbc/CMakeLists.txt
4–5

nit: extra tabs

flang/unittests/Optimizer/CMakeLists.txt
11

Why not add LLVMTargetParser here?

llvm/include/llvm/ADT/Triple.h
15

I think it would make sense to make it explicit in the comment that the header in llvm/ADT/Triple.h is deprecated.

llvm/include/llvm/Support/AArch64TargetParser.h
10

Same here - I'd make it explicit in the comment that we are going to deprecate this header.

llvm/include/llvm/Support/ARMTargetParser.h
10

Same here, make it explicit that the header will be deprecated.

llvm/include/llvm/Support/CSKYTargetParser.h
10

Same, explicit deprecation.

fpetrogalli added inline comments.Nov 16 2022, 12:53 PM
llvm/include/llvm/Support/Host.h
10

Explicit deprecation.

llvm/include/llvm/Support/LoongArchTargetParser.h
10

Same, explicit deprecation.

llvm/include/llvm/Support/RISCVISAInfo.h
10

Same, explicit deprecation.

llvm/include/llvm/Support/TargetParser.h
14

Explicit deprecation.

llvm/include/llvm/Support/X86TargetParser.def
10

Again, explicit deprecation.

llvm/include/llvm/Support/X86TargetParser.h
10

Guess! Explicit deprecation!

llvm/include/llvm/TargetParser/AArch64TargetParser.def
1–9

nit: The changes in this file are cosmetic and not needed as part of this change. I think you should submit them separately.

llvm/include/llvm/TargetParser/ARMTargetParser.h
82

II don't think this change is needed? AFAIU, the code will compile anyway even without it because the current folder is the folder where the compiler first looks for include headers?

I think you should revert this one (and the similar changes below).

If there is a real need to change it, you can do it with a new separate submission.

llvm/include/llvm/TargetParser/CSKYTargetParser.h
130

Nit: like before, this change doesn't seem to be needed.

llvm/lib/TargetParser/AArch64TargetParser.cpp
128

nit: unrelated.

llvm/lib/TargetParser/ARMTargetParser.cpp
184–185

Tabs? Unrelated.

187

nit: unrelated

204

nit: unrelated

llvm/lib/TargetParser/Unix/Host.inc
24–26

Why is the guard needed?

llvm/unittests/Support/CMakeLists.txt
3

Is this needed? The tests for TargetParser should not be here anymore, no?

yota9 removed a subscriber: yota9.Nov 16 2022, 12:54 PM
lenary updated this revision to Diff 477994.Nov 25 2022, 9:38 AM
lenary marked 23 inline comments as done.
lenary added inline comments.Nov 25 2022, 9:47 AM
clang/lib/Lex/CMakeLists.txt
3–6

Not sure, but I still think getting the case right is useful in this patch.

flang/tools/bbc/CMakeLists.txt
4–5

I've undone the re-indent here.

flang/unittests/Optimizer/CMakeLists.txt
11

That's better, done.

llvm/include/llvm/ADT/Triple.h
15

Agreed, and this needs a full LLVM header, like I did for the other forwarding headers.

llvm/lib/TargetParser/ARMTargetParser.cpp
184–185

Not tabs, just a re-dent because I clang-formatted my changes and it didn't cope with the file move. Undone.

llvm/lib/TargetParser/Unix/Host.inc
24–26

The guard is the same guard for the include that is in Unix.h in support. The problem is Unix.h in support is in a subdirectory that I cannot easily add to the include path for this file, so instead I directly copied it over.

llvm/unittests/Support/CMakeLists.txt
3

This is needed because the tests in Support still use the Triple - they are not testing the triple, they just choose whether to run the tests based on the triple.

As for the name TargetParser. @arsenm came up with the name SubtargetRegistry. I am fine with either names.

@fpetrogalli I'm going to stick with TargetParser, because I think this is less confusing, given that TargetRegistry is already a component.

The name llvm/lib/TargetParser LGTM.

fpetrogalli accepted this revision.Nov 28 2022, 2:20 AM

Thank you for working on this @lenary - LGTM!

This revision is now accepted and ready to land.Nov 28 2022, 2:20 AM
lenary updated this revision to Diff 479013.Nov 30 2022, 10:24 AM
MaskRay accepted this revision.Nov 30 2022, 8:01 PM

Please check these builds all work:

  • default
  • -DLLVM_LINK_LLVM_DYLIB=on
  • -DBUILD_SHARED_LIBS=on
lenary updated this revision to Diff 483487.Dec 16 2022, 4:04 AM
lenary retitled this revision from [RFC][Support] Move TargetParsers to new component to [Support] Move TargetParsers to new component.
lenary edited the summary of this revision. (Show Details)

The most recent update is all the fixes needed after the builds @MaskRay asked me for. I think this is ready to land on Monday?

@thakis there will be GN fallout from this change. I do not intend to update GN in this patchset, but wanted to give you a heads-up.

fpetrogalli accepted this revision.Dec 16 2022, 4:31 AM

[...] I think this is ready to land on Monday?

SGTM - thank you again!

This revision was landed with ongoing or failed builds.Dec 20 2022, 3:06 AM
This revision was automatically updated to reflect the committed changes.
lenary added a comment.EditedDec 20 2022, 3:33 AM

This caused an issue in the libc benchmarks, fix here: https://reviews.llvm.org/rG2a261a7b5764 (committed directly to main)

barannikov88 added inline comments.Dec 20 2022, 3:40 AM
llvm/include/llvm/ADT/Triple.h
11

Invalid comment.

lenary marked an inline comment as done.Dec 20 2022, 4:15 AM
lenary added inline comments.
llvm/include/llvm/ADT/Triple.h
11
lenary marked an inline comment as done.Dec 20 2022, 4:53 AM

Another fixup for the llvm examples was required, landed in https://reviews.llvm.org/rG16c4c4e04c14

And a fixup for the compiler-rt symbolizer: https://reviews.llvm.org/rGecaab107e4d0

Thanks for the heads-up. I updated the GN build in 4ac51dd53d93b8dd18c58093766483c657fe3a08 and 2aa998d22fe09191cd6c1b697e373266c1131502. The latter commit has a python script in the commit message that might be useful to others who want to port this to other build systems.

Thanks for the heads-up. I updated the GN build in 4ac51dd53d93b8dd18c58093766483c657fe3a08 and 2aa998d22fe09191cd6c1b697e373266c1131502. The latter commit has a python script in the commit message that might be useful to others who want to port this to other build systems.

Thanks! Much appreciated!

Can you please update llvm/include/llvm/module.modulemap for this change or revert the patch? This is breaking all bots that build with -DLLVM_ENABLE_MODULES=On.

For example: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/lastFailedBuild/consoleFull#111065754949ba4694-19c4-4d7e-bec5-911270d8a58c

Thanks!

Can you please update llvm/include/llvm/module.modulemap for this change or revert the patch? This is breaking all bots that build with -DLLVM_ENABLE_MODULES=On.

For example: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/lastFailedBuild/consoleFull#111065754949ba4694-19c4-4d7e-bec5-911270d8a58c

Looking at this now. Thanks for pointing it out as I didn't get a notification for this.

I think I fixed it in a685bb8e333e, but please take another look.

Also needed a follow up fix to completely fix module 9cd6fbee7ed8

I'm working on a follow-up, which should make the split a bit clearer, but I'm also not a modulemap expert and the -DLLVM_ENABLE_MODULES=On configuration is broken on my linux dev box. I'll post it for review if those two patches have at least made the build greener.

I'm working on a follow-up, which should make the split a bit clearer, but I'm also not a modulemap expert and the -DLLVM_ENABLE_MODULES=On configuration is broken on my linux dev box. I'll post it for review if those two patches have at least made the build greener.

Here: https://reviews.llvm.org/D140420 - I'm not sure how urgent this is, if your patches did get your builds green. I think in e.g. the bazel build, there's still a bunch of places where TargetParser is closely entwined with Support that we can fix in the same way we intend to fix the forwarding headers.

This has introduced a circular dependency due to the forwarding header (forwarding header depends on new lib, new lib depends on support, where the forwarding header is). Generally this wouldn't be acceptable (& I'd suggest the patch be reverted on that basis) though I understand this is a complicated migration - what's the timeline for correcting this issue?

This has introduced a circular dependency due to the forwarding header (forwarding header depends on new lib, new lib depends on support, where the forwarding header is). Generally this wouldn't be acceptable (& I'd suggest the patch be reverted on that basis) though I understand this is a complicated migration - what's the timeline for correcting this issue?

IIUC, the fix here would be to remove the forwarding headers, right?

Francesco

This has introduced a circular dependency due to the forwarding header (forwarding header depends on new lib, new lib depends on support, where the forwarding header is). Generally this wouldn't be acceptable (& I'd suggest the patch be reverted on that basis) though I understand this is a complicated migration - what's the timeline for correcting this issue?

A commit was landed to add TargetParser to the module map so it looks like part of Support, which solves the circular dependency issue in the short term, I believe.

https://reviews.llvm.org/D140420 is a proposed fix which should more clearly split the TargetParser from Support, but I'm not super familiar with modules and my linux dev environment does not work with a modules build. I'd appreciate any comments or guidance though I also know it needs an update.

Hi, we're seeing some build failures after this change. Seems like the TargetParser isn't getting added to LLVMExports correctly. Can you take a look and address these?

Failing bot https://ci.chromium.org/ui/p/fuchsia/builders/prod/llvm-linux-x64/b8794232527428765009/overview

CMAKE invocation can be found here: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8794232527428765009/+/u/configure_x86_64-linux-gnu_llvm/execution_details

Error message:

CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMCore" which requires target "LLVMTargetParser" that is not in the export set.
CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMBinaryFormat" which requires target "LLVMTargetParser" that is not in the export set.
CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMBitReader" which requires target "LLVMTargetParser" that is not in the export set.
CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMMC" which requires target "LLVMTargetParser" that is not in the export set.
CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMMCParser" which requires target "LLVMTargetParser" that is not in the export set.
CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMMCDisassembler" which requires target "LLVMTargetParser" that is not in the export set.
CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMObject" which requires target "LLVMTargetParser" that is not in the export set.
CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMDebugInfoDWARF" which requires target "LLVMTargetParser" that is not in the export set.
CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMSymbolize" which requires target "LLVMTargetParser" that is not in the export set.
CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMX86Desc" which requires target "LLVMTargetParser" that is not in the export set.
CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMAArch64Desc" which requires target "LLVMTargetParser" that is not in the export set.
CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMProfileData" which requires target "LLVMTargetParser" that is not in the export set.
CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMTextAPI" which requires target "LLVMTargetParser" that is not in the export set.

Sorry for the noise, I missed that that bot is setting a CMake flag that affects LLVM_EXPORTS. Please disregard my earlier message. I just need to update the CMake invocation to fix this, so I don't think there is anything required on your end.

zixuan-wu added inline comments.
llvm/include/llvm/Support/RISCVISAInfo.h
10

I think there is a typo that should be 'in favour of llvm/TargetParser/RISCVISAInfo.h '

llvm/include/llvm/Support/RISCVISAInfo.h
10

I may move RISCVISAInfo back to Support and fix this in D140529.

Thanks for letting me know. I'm back in the office on the 3rd, and it looks like this will be top of my list to look at, not that I'm very familiar with the pass and what it's up to.