[PATCH] D137516: [TargetSupport] Move TargetParser API in a separate LLVM component.

Fri Nov 11 07:16:29 PST 2022

lenary added a comment.

In D137516#3912900 <https://reviews.llvm.org/D137516#3912900>, @fpetrogalli wrote:

> In D137516#3912842 <https://reviews.llvm.org/D137516#3912842>, @lenary wrote:
>
>> In D137516#3912751 <https://reviews.llvm.org/D137516#3912751>, @fpetrogalli wrote:
>>
>>> @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 `MVT`s, 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
>
>>> 2. 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.
>
>>> 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`?
>>
>> 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137516/new/

https://reviews.llvm.org/D137516