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

Mon Aug 10 08:59:34 PDT 2020

yaxunl marked 2 inline comments as done.
yaxunl added a comment.

In D80750#2183604 <https://reviews.llvm.org/D80750#2183604>, @jdoerfert wrote:

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

There are different kinds of target features. Some features are for optimization purpose, therefore a module compiled with such a feature on can be linked with a module with such feature off without problem. For such features, our target ID rule for feature matching does not apply, since we do not allow a module with a feature on to be linked with the same feature off.

Not all features are allowed in target ID. The features allowed in target ID have special traits and requirements:

1. They correspond to a processor configuration which cannot be changed dynamically
2. The ISA generated with such feature on can only be loaded on a processor with such feature on. The ISA generated with such feature off can only be loaded on a processor with such a feature off. That's why there is a rule that a module with such a feature on can not be linked with a module with the same feature off.
3. It is required such features have a default value which works with the processor configured in either way. If the feature is not specified in target ID, it takes the default value. Therefore a LLVM module compiled without such explicit feature can be linked with a LLVM module compiled with this feature explicitly on/off. This is not generally true for arbitrary features.

In a summary, when a feature shows up in target ID, there is guarantee that they satisfy these traits and requirements, therefore they need the matching/merging rule that is defined for target ID. Whereas an arbitrary feature does not satisfy these traits and requirements.

Basically the features in target ID together with processor are used to identify the processor and its configuration for what an LLVM module is compiled for. As such, it is better to be as one entity, instead of split up as processor and features.

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

I think representing target ID at module level in a similar way like triple is a good idea, since this will make it compatible with heterogeneous IR.

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

https://reviews.llvm.org/D80750