IR Linking: Support merging Warning+Max module metadata flags

[dblaikie]

Summary

Debug Info Version was changed to use "Max" instead of "Warning" per the
original design intent - but this maxes old/new IR unlinkable, since
mismatched merge styles are a linking failure.

It seems possible/maybe reasonable to actually support the combination
of these two flags: Warn, but then use the maximum value rather than the
first value/earlier module's value.

To fix: https://bugs.llvm.org/show_bug.cgi?id=44784

[tejohnson]

LGTM with a couple minor suggestions below.
I think this makes sense.

llvm/lib/Linker/IRMover.cpp

1377

A comment about why this isn't in the switch statement would be helpful, along with a note about how the new Flag is formed.

Also, it might be clearer to put this before the switch, after the new warning handling, with a continue to skip the then unnecessary switch (it consolidates the special case handling for Warning+Max).

[dblaikie]

llvm/lib/Linker/IRMover.cpp

1377

Makes sense, thanks! Did those.

llvm/docs/LangRef.rst

.. list-table::
    * - 1
      - **Error**
            Emits an error if two values disagree, otherwise the resulting value
            is that of the operands.
 
    * - 2
      - **Warning**
            Emits a warning if two values disagree. The result value will be the
-           operand for the flag from the first module being linked.
+           operand for the flag from the first module being linked, or the max
+           if the other module uses **Max** (in which case the resulting flag
+           will be **Max**).
 
    * - 3
      - **Require**
            Adds a requirement that another module flag be present and have a
            specified value after linking is performed. The value must be a
            metadata pair, where the first element of the pair is the ID of the
            module flag to be restricted, and the second element of the pair is
            the value the module flag should be restricted to. This behavior can

llvm/lib/Linker/IRMover.cpp

if (DstBehaviorValue == Module::Override) {
       continue;
     } else if (SrcBehaviorValue == Module::Override) {
       // Update the destination flag to that of the source.
       overrideDstValue();
       continue;
     }
 
     // Diagnose inconsistent merge behavior types.
-    if (SrcBehaviorValue != DstBehaviorValue)
+    if (SrcBehaviorValue != DstBehaviorValue) {
+      bool MaxAndWarn = (SrcBehaviorValue == Module::Max &&
+                         DstBehaviorValue == Module::Warning) ||
+                        (DstBehaviorValue == Module::Max &&
+                         SrcBehaviorValue == Module::Warning);
+      if (!MaxAndWarn)
         return stringErr("linking module flags '" + ID->getString() +
                          "': IDs have conflicting behaviors in '" +
                          SrcM->getModuleIdentifier() + "' and '" +
                          DstM.getModuleIdentifier() + "'");
+    }
 
     auto replaceDstValue = [&](MDNode *New) {
       Metadata *FlagOps[] = {DstOp->getOperand(0), ID, New};
       MDNode *Flag = MDNode::get(DstM.getContext(), FlagOps);
       DstModFlags->setOperand(DstIndex, Flag);
       Flags[ID].first = Flag;
     };
 
+    // Emit a warning if the values differ and either source or destination
+    // request Warning behavior.
+    if ((DstBehaviorValue == Module::Warning ||
+         SrcBehaviorValue == Module::Warning) &&
+        SrcOp->getOperand(2) != DstOp->getOperand(2)) {
+      std::string Str;
+      raw_string_ostream(Str)
+          << "linking module flags '" << ID->getString()
+          << "': IDs have conflicting values ('" << *SrcOp->getOperand(2)
+          << "' from " << SrcM->getModuleIdentifier() << " with '"
+          << *DstOp->getOperand(2) << "' from " << DstM.getModuleIdentifier()
+          << ')';
+      emitWarning(Str);
+    }
+
+    // Choose the maximum if either source or destination request Max behavior.
+    if (DstBehaviorValue == Module::Max || SrcBehaviorValue == Module::Max) {
+      ConstantInt *DstValue =
+          mdconst::extract<ConstantInt>(DstOp->getOperand(2));
+      ConstantInt *SrcValue =
+          mdconst::extract<ConstantInt>(SrcOp->getOperand(2));
+
+      // The resulting flag should have a Max behavior, and contain the maximum
+      // value from between the source and destination values.
+      Metadata *FlagOps[] = {
+          (DstBehaviorValue != Module::Max ? SrcOp : DstOp)->getOperand(0), ID,
+          (SrcValue->getZExtValue() > DstValue->getZExtValue() ? SrcOp : DstOp)
+              ->getOperand(2)};
+      MDNode *Flag = MDNode::get(DstM.getContext(), FlagOps);
+      DstModFlags->setOperand(DstIndex, Flag);
+      Flags[ID].first = Flag;
+      continue;
+    }
+
     // Perform the merge for standard behavior types.
     switch (SrcBehaviorValue) {
     case Module::Require:
     case Module::Override:
       llvm_unreachable("not possible");
     case Module::Error: {
       // Emit an error if the values differ.
       if (SrcOp->getOperand(2) != DstOp->getOperand(2))
         return stringErr("linking module flags '" + ID->getString() +
                          "': IDs have conflicting values in '" +
                          SrcM->getModuleIdentifier() + "' and '" +
                          DstM.getModuleIdentifier() + "'");
       continue;
     }
     case Module::Warning: {
-      // Emit a warning if the values differ.
-      if (SrcOp->getOperand(2) != DstOp->getOperand(2)) {
-        std::string str;
-        raw_string_ostream(str)
-            << "linking module flags '" << ID->getString()
-            << "': IDs have conflicting values ('" << *SrcOp->getOperand(2)
-            << "' from " << SrcM->getModuleIdentifier() << " with '"
-            << *DstOp->getOperand(2) << "' from " << DstM.getModuleIdentifier()
-            << ')';
-        emitWarning(str);
-      }
-      continue;
+      break;
     }
     case Module::Max: {
-      ConstantInt *DstValue =
-          mdconst::extract<ConstantInt>(DstOp->getOperand(2));
-      ConstantInt *SrcValue =
-          mdconst::extract<ConstantInt>(SrcOp->getOperand(2));
-      if (SrcValue->getZExtValue() > DstValue->getZExtValue())
-        overrideDstValue();
       break;
     }
     case Module::Append: {
       MDNode *DstValue = cast<MDNode>(DstOp->getOperand(2));
       MDNode *SrcValue = cast<MDNode>(SrcOp->getOperand(2));
       SmallVector<Metadata *, 8> MDs;
       MDs.reserve(DstValue->getNumOperands() + SrcValue->getNumOperands());
       MDs.append(DstValue->op_begin(), DstValue->op_end());
case Module::AppendUnique: {
       Elts.insert(DstValue->op_begin(), DstValue->op_end());
       Elts.insert(SrcValue->op_begin(), SrcValue->op_end());
 
       replaceDstValue(MDNode::get(DstM.getContext(),
                                   makeArrayRef(Elts.begin(), Elts.end())));
       break;
     }
     }
+
   }
 
   // Check all of the requirements.
   for (unsigned I = 0, E = Requirements.size(); I != E; ++I) {
     MDNode *Requirement = Requirements[I];
     MDString *Flag = cast<MDString>(Requirement->getOperand(0));
     Metadata *ReqValue = Requirement->getOperand(1);
 
     MDNode *Op = Flags[Flag].first;

llvm/test/Linker/Inputs/module-max-warn.ll

+!llvm.module.flags = !{!0, !1}
+!0 = !{i32 2, !"Combine Max and Warn", i32 4}
+!1 = !{i32 7, !"Combine Warn and Max", i32 5}

llvm/test/Linker/module-max-warn.ll

+; RUN: llvm-link %s %p/Inputs/module-max-warn.ll -S -o - 2>&1 | FileCheck %s
+
+; CHECK: warning: linking module flags 'Combine Max and Warn': IDs have conflicting values ('i32 4' from {{.*}}/test/Linker/Inputs/module-max-warn.ll with 'i32 2' from llvm-link)
+; CHECK: warning: linking module flags 'Combine Warn and Max': IDs have conflicting values ('i32 5' from {{.*}}/test/Linker/Inputs/module-max-warn.ll with 'i32 3' from llvm-link)
+
+
+; CHECK: !0 = !{i32 7, !"Combine Max and Warn", i32 4}
+; CHECK: !1 = !{i32 7, !"Combine Warn and Max", i32 5}
+
+!llvm.module.flags = !{!0, !1}
+!0 = !{i32 7, !"Combine Max and Warn", i32 2}
+!1 = !{i32 2, !"Combine Warn and Max", i32 3}