[PATCH] D80750: llvm-link: Add module flag behavior MergeTargetID

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D80750#2181709 <https://reviews.llvm.org/D80750#2181709>, @yaxunl wrote:

> In D80750#2180284 <https://reviews.llvm.org/D80750#2180284>, @jdoerfert wrote:
>
>> I believe there are three things in this patch, but feel free to correct me:
>>
>> 1. A way to specify a target triple + cpu. Basically like `target triple = ...` but in the module metadata plus some additional target cpu suffix, which is so far in the `target-cpu` function attribute list.
>> 2. A way to specify global target features, which are so far in the `target-features` function attribute list.
>> 3. Making llvm-link aware of 1) and 2) and verifying they match (under some rules).
>>
>> If this is the case, what is the benefit over a toplevel module entry that allows you to specify `target-cpu` and `target-feature` for the entire module? I mean, they seem to be very much the same thing as `target triple`, yet we go a totally different route to add them and verify the match during linking. I believe other people might benefit from this, e.g., to get rid of function-level attributes, so we should shoot for a generic solution.
>
> The matching and merging rule we want to apply to the target features in target ID is not generic for arbitrary target features. We can apply these rules to target features only if they appear in a target ID.

Could you elaborate why this doesn't apply to arbitrary target features? Where and how is decided what is an OK feature and how they are merged? When I look at the merging procedure in the `llvm/lib/Linker/IRMover.cpp` I don't see any code that is not generic and applies to arbitrary target feature. It looks like you say a feature present in both needs to have the same sign and the union of features is used for the resulting module, correct? I would assume I can write `+foobar` in one or two module "target-id" metadata and they will be merged just fine with this patch, is this wrong?

> Also we do not see strong motivation to enforce matching of target-cpu in general, otherwise such a rule should have been added to the linker. However, for target ID we have reasons to enforce such checking.

If you have them named in global scope and allow each global symbol to reference one, you don't have to enforce matching in general or at all. But you can choose to require at most one (or compatible ones) for the AMDGPU backend. This patch has a single rule that suits your needs now, target-cpu is module wide and has to match, which is way less flexible.

> target ID is documented in https://reviews.llvm.org/D84822 It is general enough to be adopted by any targets if it is needed. In the degenerated case it can be simply the processor, in more general case it can includes target features that need to be consistent across modules. The usage of target-id module flag is like a contract saying that what needs to be matched, whereas in general such matching is not required.

All of this would be true for (named) global target id/features on module level as well, wouldn't it? What makes this a module flag thing as opposed to be at the same level with target triple? Especially if we have other reasons to have multiple target triples anyway. This is adding a completely new scheme with hardcoded rules in a completely different way than the existing scheme works.


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

https://reviews.llvm.org/D80750