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

[Scott Linder via Phabricator via llvm-commits]

scott.linder 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
----------------
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?


================
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
----------------
Can this explicitly mention that mismatched triple-cpu causes an error?


================
Comment at: llvm/lib/Linker/IRMover.cpp:1207
+      ConstantInt *Behavior = mdconst::extract<ConstantInt>(Op->getOperand(0));
+      if (Behavior->getZExtValue() == Module::MergeTargetId)
+        return true;
----------------
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? 


================
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();
----------------
Can these be explicitly typed as `StringRef`? It wasn't clear to me reading it the first time, especially seeing the cast first.


================
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) {
----------------
Can this mention that the first ':' delimits the "<triple>-<cpu>" portion from the features, which are then also delimited by ':'?


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


================
Comment at: llvm/lib/Linker/IRMover.cpp:1468
+        if (Loc == DstFeatures.end()) {
+          if (DstTripleCPU != SrcTripleCPU)
+            return stringErr("linking module flags '" + ID->getString() +
----------------
Why is this done nested within the merging of features; can this instead be done once, early on?


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

https://reviews.llvm.org/D80750