This is an archive of the discontinued LLVM Phabricator instance.

[TargetSupport] Move TargetParser API in a separate LLVM component.
AbandonedPublic

Authored by fpetrogalli on Nov 6 2022, 5:13 PM.

Details

Reviewers
NoQ
craig.topper

Diff Detail

Event Timeline

fpetrogalli created this revision.Nov 6 2022, 5:13 PM
Herald added a project: Restricted Project. · View Herald Transcript
fpetrogalli requested review of this revision.Nov 6 2022, 5:13 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 6 2022, 5:13 PM
arsenm added a comment.Nov 6 2022, 9:24 PM

I think this is fine, but think the name needs to be bikeshedded. "Target" and "Support" are already overloaded enough as it is. Is there anything else that would really ever go into this library?

I think this is fine, but think the name needs to be bikeshedded. "Target" and "Support" are already overloaded enough as it is. Is there anything else that would really ever go into this library?

I was wondering this new library could house MVTs in some form. I've wanted to auto-generate both the C++ and tablegen MVTs from a single source of truth for a while.

lenary added a subscriber: lenary.Nov 7 2022, 7:44 AM

I'm not convinced we should be leaving any of the other Target Parser information in Support, if we are doing this, though this creates layering issues which I've been trying to unpick.

If you try to take llvm/Support/*TargetParser*, you find a few places where there are dependencies from these files to other bits of Support:

  • llvm/Support/Host has lots of logic for turning cpu information into a target, to handle -mcpu=native. My opinion is that should also move into target parser. There are some parts of this file that can move into other parts of llvm/Support, like the introspection about number of cores.
  • llvm/{Support/ADT}Triple depends on the Arm/AArch64 target parsers (because arm triples canonically look like armv7a-… or similar, and we use the targetparser to help canonicalise the triple). My opinion is that all this code can move into the target parser.

There are quite a few small cleanup patches to enable this. One of the biggest is that llvm/Support/CommmandLine wants to know the current triple and CPU for --version, even in e.g. tablegen. This was the change I most struggled with, but I think I have a solution that does the job fine.

I'm going to try to polish and post my patch set this week which does this split, as a comparison to this one.

Re the name: "TargetParser" seems reasonable to me, given that's what we've called the files so far anyway.

@arsenm @frasercrmck @lenary - thank you for the feedback

  1. The library could host anything that needs to be auto-generated with tablegen. If we add MVTs, the name TargetSupport becomes obsolete.
  2. Therefore we could use a name like AutoGenSupport (or similar, I am open to suggestions).
  3. @lenary is saying that components like Support/Host needs to be moved together with the AArch64/ARM bits of the target parser. Do people see an issue if we add files that do not need auto-generated content in a library called AutoGenSupport?
lenary added a comment.EditedNov 7 2022, 9:58 AM

@arsenm @frasercrmck @lenary - thank you for the feedback

  1. The library could host anything that needs to be auto-generated with tablegen. If we add MVTs, the name TargetSupport becomes obsolete.

I am in favour of splitting parts of Support that could be table-gen'd out of Support, and into supplemental libraries.

  1. Therefore we could use a name like AutoGenSupport (or similar, I am open to suggestions).

I actually think this is an awful name, even worse than "Support". One of the big issues with "Support" is that it's a junk-drawer that anyone throws stuff in when they need it anywhere in LLVM, when their layering questions become more difficult. This then becomes very hard to undo, because everything at this layer depends on everything else. "AutoGenSupport" to me says "we now have two bits, the junk drawer, and the junk drawer that's auto generated" and so I feel this is is worse, as you cannot differentiate which you need based on what you're doing.

I think we need to name things after what they're for, not how they're made.

  1. @lenary is saying that components like Support/Host needs to be moved together with the AArch64/ARM bits of the target parser. Do people see an issue if we add files that do not need auto-generated content in a library called AutoGenSupport?

The reason I'm in favour of "TargetParser" is because we already call the classes and related files "target parsers", and most of them contain target-related parsers. I don't know why this is a bad name.

Edited to add: Once I've split out Support/Host, I'm likely to give it a less generic name, something like "HostDetection", which is more clearly what it is doing.

@arsenm @frasercrmck @lenary - thank you for the feedback

  1. The library could host anything that needs to be auto-generated with tablegen. If we add MVTs, the name TargetSupport becomes obsolete.

I am in favour of splitting parts of Support that could be table-gen'd out of Support, and into supplemental libraries.

+1

  1. Therefore we could use a name like AutoGenSupport (or similar, I am open to suggestions).

I actually think this is an awful name, even worse than "Support". One of the big issues with "Support" is that it's a junk-drawer that anyone throws stuff in when they need it anywhere in LLVM, when their layering questions become more difficult. This then becomes very hard to undo, because everything at this layer depends on everything else. "AutoGenSupport" to me says "we now have two bits, the junk drawer, and the junk drawer that's auto generated" and so I feel this is is worse, as you cannot differentiate which you need based on what you're doing.

I think we need to name things after what they're for, not how they're made.

Agree.

  1. @lenary is saying that components like Support/Host needs to be moved together with the AArch64/ARM bits of the target parser. Do people see an issue if we add files that do not need auto-generated content in a library called AutoGenSupport?

The reason I'm in favour of "TargetParser" is because we already call the classes and related files "target parsers", and most of them contain target-related parsers. I don't know why this is a bad name.

I'd be very happy to use the name TargetParser. I initially used that because I knew that moving the RSCV bits of it meant we needed to move the other parsers too, together with Support/Host. Therefore I was trying to create a name that would have covered for both the target parser any other pieces that would have been moved in it. However...

Edited to add: Once I've split out Support/Host, I'm likely to give it a less generic name, something like "HostDetection", which is more clearly what it is doing.

... you seem to imply that you want to create a second component that is independent of [TargetParser|TargetSUpport]. If that's the case, I am even more convinced that TargetParser is the best name for the component introduced in this patch.

@arsenm @frasercrmck @lenary - thank you for the feedback

  1. The library could host anything that needs to be auto-generated with tablegen. If we add MVTs, the name TargetSupport becomes obsolete.

I am in favour of splitting parts of Support that could be table-gen'd out of Support, and into supplemental libraries.

+1

  1. Therefore we could use a name like AutoGenSupport (or similar, I am open to suggestions).

I actually think this is an awful name, even worse than "Support". One of the big issues with "Support" is that it's a junk-drawer that anyone throws stuff in when they need it anywhere in LLVM, when their layering questions become more difficult. This then becomes very hard to undo, because everything at this layer depends on everything else. "AutoGenSupport" to me says "we now have two bits, the junk drawer, and the junk drawer that's auto generated" and so I feel this is is worse, as you cannot differentiate which you need based on what you're doing.

I think we need to name things after what they're for, not how they're made.

Agree.

  1. @lenary is saying that components like Support/Host needs to be moved together with the AArch64/ARM bits of the target parser. Do people see an issue if we add files that do not need auto-generated content in a library called AutoGenSupport?

The reason I'm in favour of "TargetParser" is because we already call the classes and related files "target parsers", and most of them contain target-related parsers. I don't know why this is a bad name.

I'd be very happy to use the name TargetParser. I initially used that because I knew that moving the RSCV bits of it meant we needed to move the other parsers too, together with Support/Host. Therefore I was trying to create a name that would have covered for both the target parser any other pieces that would have been moved in it. However...

Edited to add: Once I've split out Support/Host, I'm likely to give it a less generic name, something like "HostDetection", which is more clearly what it is doing.

... you seem to imply that you want to create a second component that is independent of [TargetParser|TargetSUpport]. If that's the case, I am even more convinced that TargetParser is the best name for the component introduced in this patch.

The patch that outlines my proposal for TargetParser is here: https://reviews.llvm.org/D137838 (it has dependent patches). I also added a comment to your RFC thread explaining the dependent patches.

fpetrogalli abandoned this revision.Dec 20 2022, 8:24 AM

Replaced by D137838

clang/unittests/Basic/CMakeLists.txt