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

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl added inline comments.


================
Comment at: llvm/docs/LangRef.rst:6451
+     - **MergeTargetId**
+           Merge two string values in target id format. A target id consists of
+           a triple-cpu id string followed by a list of features delimited by
----------------
scott.linder wrote:
> Do we have a canonical definition of "TargetID" somewhere? I know it isn't updated yet, but would https://llvm.org/docs/AMDGPUUsage.html#code-object-target-identification be a reasonable place?
> 
> The format is general enough for any target to use, so maybe it should be described somewhere else and linked to from AMDGPUUsage?
I will update AMDGPUUsage to include definition of target ID and add a link to that.

I am not sure if there is a better place for target ID definition. It is a concept that are used in AMDGPU code object bundle, LLVM IR module flag, and clang option. Consider these three places, it seems AMDGPUusage is best since it is the place where the concept is originated.


================
Comment at: llvm/docs/LangRef.rst:6455
+           in the source target id only are added to the destination target id
+           if their triple-cpu id matches. If a feature is in both source and
+           destination target ids but with different sign, an error of conflict
----------------
scott.linder wrote:
> Can this explicitly mention that mismatched triple-cpu causes an error?
done


================
Comment at: llvm/lib/Linker/IRMover.cpp:1207
+      ConstantInt *Behavior = mdconst::extract<ConstantInt>(Op->getOperand(0));
+      if (Behavior->getZExtValue() == Module::MergeTargetId)
+        return true;
----------------
scott.linder wrote:
> Shouldn't the second operand, the "unique ID" of the metadata, be considered too? There could be many unrelated metadata flags which all use the MergeTargetID behavior, and this seems like it will conflate them. Could you add a test for this case too? 
since this module flag behavior is only used for target ID, I specified in the documentation that the key must be `target-id` if the module flag behavior is MergeTargetID, and added check to verifier to make sure the key is `target-id` when module flag behavior is MergeTargetID. Also added lit test.


================
Comment at: llvm/lib/Linker/IRMover.cpp:1409
+    case Module::MergeTargetId: {
+      auto DstTargetId = cast<MDString>(DstOp->getOperand(2))->getString();
+      auto SrcTargetId = cast<MDString>(SrcOp->getOperand(2))->getString();
----------------
scott.linder wrote:
> Can these be explicitly typed as `StringRef`? It wasn't clear to me reading it the first time, especially seeing the cast first.
done


================
Comment at: llvm/lib/Linker/IRMover.cpp:1413
+      // Extract features from target id. Features are strings ending with '+'
+      // or '-' delimited by ':'. Returns false if the format is invalid.
+      auto GetFeatures = [](StringRef TargetId, StringMap<bool> &Features) {
----------------
scott.linder wrote:
> Can this mention that the first ':' delimits the "<triple>-<cpu>" portion from the features, which are then also delimited by ':'?
done


================
Comment at: llvm/lib/Linker/IRMover.cpp:1454
+          return stringErr("linking module flags '" + ID->getString() +
+                           "': IDs have conflicting values ('" + DstTargetId +
+                           "' from '" + SrcM->getModuleIdentifier() +
----------------
scott.linder wrote:
> Could you use either id or ID everywhere consistently, including comments? I would prefer ID
done


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

https://reviews.llvm.org/D80750