[MLIR][mlir-link] Fix issues in mlir link path

Author: frabert

mlir/include/mlir/Dialect/LLVMIR/LLVMLinkerInterface.h

@@ -50,7 +50,7 @@ class LLVMSymbolLinkerInterface
     for (auto op : toLink) {
       auto structor = dyn_cast<structor_t>(op);
       if (!structor)
-        llvm_unreachable("invali global structor operation");
+        llvm_unreachable("invalid global structor operation");
 
       ArrayRef<Attribute> structorList;
       if constexpr (std::is_same<LLVM::GlobalCtorsOp, structor_t>()) {

mlir/include/mlir/Linker/LLVMLinkerMixin.h

@@ -340,29 +340,38 @@ class LLVMLinkerMixin {
       return ConflictResolution::LinkFromSrc;
     }
 
-    std::optional<ComdatSelector> srcComdatSel = derived.getComdatSelector(pair.src);
-    std::optional<ComdatSelector> dstComdatSel = derived.getComdatSelector(pair.dst);
+    std::optional<ComdatSelector> srcComdatSel =
+        derived.getComdatSelector(pair.src);
+    std::optional<ComdatSelector> dstComdatSel =
+        derived.getComdatSelector(pair.dst);
     if (srcComdatSel.has_value() && dstComdatSel.has_value()) {
       auto srcComdatName = srcComdatSel->name;
       auto dstComdatName = dstComdatSel->name;
       auto srcComdat = srcComdatSel->kind;
       auto dstComdat = dstComdatSel->kind;
       if (srcComdatName != dstComdatName) {
-          llvm_unreachable("Comdat selector names don't match");
+        llvm_unreachable("Comdat selector names don't match");
       }
       if (srcComdat != dstComdat) {
-          llvm_unreachable("Comdat selector kinds don't match");
+        llvm_unreachable("Comdat selector kinds don't match");
       }
 
       if (srcComdat == mlir::LLVM::comdat::Comdat::Any) {
-          return ConflictResolution::LinkFromDst;
+        return ConflictResolution::LinkFromDst;
       }
       if (srcComdat == mlir::LLVM::comdat::Comdat::NoDeduplicate) {
-          return ConflictResolution::Failure;
+        return ConflictResolution::Failure;
       }
       llvm_unreachable("unimplemented comdat kind");
     }
 
+    // If we reach here, we have two external definitions that can't be resolved
+    // This is typically an error case in LLVM linking
+    if (isExternalLinkage(srcLinkage) && isExternalLinkage(dstLinkage) &&
+        !srcIsDeclaration && !dstIsDeclaration) {
+      return ConflictResolution::Failure;
+    }
+
     llvm_unreachable("unimplemented conflict resolution");
   }
 };

mlir/lib/Dialect/LLVMIR/IR/LLVMLinkerInterface.cpp

@@ -85,23 +85,25 @@ static bool hasComdat(Operation *op) {
 static SymbolRefAttr getComdatSymbol(Operation *op) {
   assert(hasComdat(op) && "Operation with Comdat expected");
   if (auto gv = dyn_cast<LLVM::GlobalOp>(op))
-      return gv.getComdat().value();
+    return gv.getComdat().value();
   if (auto fn = dyn_cast<LLVM::LLVMFuncOp>(op))
-      return fn.getComdat().value();
+    return fn.getComdat().value();
   llvm_unreachable("unexpected operation");
 }
 
 bool LLVM::LLVMSymbolLinkerInterface::isComdat(Operation *op) {
   return isa<LLVM::ComdatOp>(op);
 }
 
-std::optional<mlir::link::ComdatSelector> LLVM::LLVMSymbolLinkerInterface::getComdatSelector(Operation *op) {
+std::optional<mlir::link::ComdatSelector>
+LLVM::LLVMSymbolLinkerInterface::getComdatSelector(Operation *op) {
   if (!hasComdat(op))
     return std::nullopt;
 
   auto symbol = getComdatSymbol(op);
   auto *symTabOp = SymbolTable::getNearestSymbolTable(op);
-  auto comdatSelector = cast<mlir::LLVM::ComdatSelectorOp>(SymbolTable::lookupSymbolIn(symTabOp, symbol));
+  auto comdatSelector = cast<mlir::LLVM::ComdatSelectorOp>(
+      SymbolTable::lookupSymbolIn(symTabOp, symbol));
   return {{comdatSelector.getSymName(), comdatSelector.getComdat()}};
 }
 
@@ -112,7 +114,8 @@ bool LLVM::LLVMSymbolLinkerInterface::isDeclaration(Operation *op) {
     return gv.getInitializerRegion().empty() && !gv.getValue();
   if (auto fn = dyn_cast<LLVM::LLVMFuncOp>(op))
     return fn.getBody().empty();
-  if (isa<LLVM::GlobalCtorsOp, LLVM::GlobalDtorsOp, LLVM::ComdatOp, LLVM::AliasOp>(op))
+  if (isa<LLVM::GlobalCtorsOp, LLVM::GlobalDtorsOp, LLVM::ComdatOp,
+          LLVM::AliasOp>(op))
     return false;
   llvm_unreachable("unexpected operation");
 }
@@ -446,48 +449,50 @@ getAppendedOpWithInitRegion(llvm::ArrayRef<mlir::Operation *> globs,
   return targetGV;
 }
 
-static Operation *appendGlobalOps(ArrayRef<Operation *> globs, LLVM::GlobalOp lastGV, LinkState &state) {
+static Operation *appendGlobalOps(ArrayRef<Operation *> globs,
+                                  LLVM::GlobalOp lastGV, LinkState &state) {
   // Src ops that are declarations are ignored in favour of dst operation
   // This mimics the behaviour of linkAppendingVarProto in llvm-link
   if (globs.size() == 1)
-      return state.clone(globs.front());
+    return state.clone(globs.front());
 
   if (!lastGV.getInitializer().empty()) {
-      return getAppendedOpWithInitRegion(globs, state);
+    return getAppendedOpWithInitRegion(globs, state);
   } else {
-      auto [value, type] = getAppendedAttr(globs, state);
+    auto [value, type] = getAppendedAttr(globs, state);
 
-      auto valueAttrName = lastGV.getValueAttrName();
-      auto typeAttrName = lastGV.getGlobalTypeAttrName();
+    auto valueAttrName = lastGV.getValueAttrName();
+    auto typeAttrName = lastGV.getGlobalTypeAttrName();
 
-      auto cloned = state.clone(globs.back());
-      cloned->setAttr(valueAttrName, value);
-      cloned->setAttr(typeAttrName, TypeAttr::get(type));
-      return cloned;
+    auto cloned = state.clone(globs.back());
+    cloned->setAttr(valueAttrName, value);
+    cloned->setAttr(typeAttrName, TypeAttr::get(type));
+    return cloned;
   }
   llvm_unreachable("unknown value attribute type");
 }
 
-static Operation *appendComdatOps(ArrayRef<Operation *> globs, LLVM::ComdatOp comdat, LinkState &state) {
+static Operation *appendComdatOps(ArrayRef<Operation *> globs,
+                                  LLVM::ComdatOp comdat, LinkState &state) {
   auto result = cast<LLVM::ComdatOp>(state.clone(comdat));
   llvm::StringMap<Operation *> selectors;
 
   for (auto selector : result.getOps<LLVM::ComdatSelectorOp>()) {
-      selectors[selector.getSymName()] = selector;
+    selectors[selector.getSymName()] = selector;
   }
 
   for (auto *glob : globs) {
     comdat = dyn_cast<LLVM::ComdatOp>(glob);
-      for (auto &op : comdat.getBody().getOps()) {
-        auto selector = cast<LLVM::ComdatSelectorOp>(op);
-        auto selectorName = selector.getSymName();
-        if (selectors.contains(selectorName)) {
-            continue;
-        }
-        auto *cloned = state.clone(selector);
-        cloned->moveBefore(&result.getBody().front().back());
-        selectors[selectorName] = cloned;
+    for (auto &op : comdat.getBody().getOps()) {
+      auto selector = cast<LLVM::ComdatSelectorOp>(op);
+      auto selectorName = selector.getSymName();
+      if (selectors.contains(selectorName)) {
+        continue;
       }
+      auto *cloned = state.clone(selector);
+      cloned->moveBefore(&result.getBody().front().back());
+      selectors[selectorName] = cloned;
+    }
   }
   return result;
 }
@@ -500,6 +505,8 @@ Operation *LLVM::LLVMSymbolLinkerInterface::appendGlobals(llvm::StringRef glob,
     return appendGlobalStructors<LLVM::GlobalDtorsOp>(state);
 
   const auto &globs = append.lookup(glob);
+  if (globs.empty())
+    return nullptr;
   if (auto lastGV = dyn_cast<LLVM::GlobalOp>(globs.back()))
     return appendGlobalOps(globs, lastGV, state);
   if (auto comdat = dyn_cast<LLVM::ComdatOp>(globs.back()))