[CIR][CIRGen] Add comdat support, enabling more dso_local and comdat LLVM tagging (llvm#751)

This PR implements:
1. Add comdat attribute as an optional attribute to CIR GlobalOp and
FuncOp, they are of the enum ComdatSelection type because it contains
all information we need. The
[llvm/include/llvm/IR/Comdat.h](https://github.com/llvm/clangir/blob/4ea2ec38638391b964e88b77f926e0892b350e04/llvm/include/llvm/IR/Comdat.h#L33)
has Comdat with selectionKind and its name, add users which are set of
pointers to GlobalObjects sharing the same name. The name is always the
name of global object (in CIR context, the GlobalOp or FuncOp), so we
don't need it. And the user set can always be collected when a
transformation looks up symbol table of the module, thus not really
necessary. Plus, it's not good for CIR to keep a set of pointers.
2. Thanks to comdat support, and adding call sites of setComdat similar
to what was in OG, we are able to implement canBenefitFromLocalAlias as
similar as possible to [GlobalValue::canBenefitFromLocalAlias()
](https://github.com/llvm/clangir/blob/4ea2ec38638391b964e88b77f926e0892b350e04/llvm/lib/IR/Globals.cpp#L112)
and this enable us to complete dso_local work by correctly setting
dso_local for functions.
3. I took back printing of dsolocal attribute of CIR, as I would fix
128+ test cases instead of 36-37 of tests (mostly adding dso_local to
function prototype line in LLVM check) in this PR. printing dsolocal
attribute is not really necessary as llvm IR would verify it for us.
4. LLVM lowering of Comdat for GlobalOp 


Things left to next/future PR
1. I'd like to extend setComdat to other parts of CG like Vtable, etc. 
2. LLVM lowering of comdat for FuncOp

Author: ghehg

clang/include/clang/CIR/Dialect/IR/CIROps.td

@@ -2082,7 +2082,7 @@ def TLSModel : I32EnumAttr<
     [TLS_GeneralDynamic, TLS_LocalDynamic, TLS_InitialExec, TLS_LocalExec]> {
   let cppNamespace = "::mlir::cir";
 }
-
+  
 def GlobalOp : CIR_Op<"global",
                       [DeclareOpInterfaceMethods<RegionBranchOpInterface>,
                        DeclareOpInterfaceMethods<CIRGlobalValueInterface>,
@@ -2123,6 +2123,7 @@ def GlobalOp : CIR_Op<"global",
                        OptionalAttr<TLSModel>:$tls_model,
                        // Note this can also be a FlatSymbolRefAttr
                        OptionalAttr<AnyAttr>:$initial_value,
+                       UnitAttr:$comdat,
                        UnitAttr:$constant,
                        UnitAttr:$dsolocal,
                        OptionalAttr<I64Attr>:$alignment,
@@ -2134,6 +2135,7 @@ def GlobalOp : CIR_Op<"global",
        ($sym_visibility^)?
        (`constant` $constant^)?
        $linkage
+       (`comdat` $comdat^)?
        ($tls_model^)?
        (`dsolocal` $dsolocal^)?
        $sym_name
@@ -2878,6 +2880,7 @@ def FuncOp : CIR_Op<"func", [
                                          "GlobalLinkageKind::ExternalLinkage">:$linkage,
                        ExtraFuncAttr:$extra_attrs,
                        OptionalAttr<StrAttr>:$sym_visibility,
+                       UnitAttr:$comdat,
                        OptionalAttr<DictArrayAttr>:$arg_attrs,
                        OptionalAttr<DictArrayAttr>:$res_attrs,
                        OptionalAttr<FlatSymbolRefAttr>:$aliasee,

clang/include/clang/CIR/Interfaces/CIROpInterfaces.td

@@ -58,6 +58,12 @@ let cppNamespace = "::mlir::cir" in {
       }]
       >,
       InterfaceMethod<"",
+      "bool", "isExternalLinkage", (ins), [{}],
+      /*defaultImplementation=*/[{ 
+        return mlir::cir::isExternalLinkage($_op.getLinkage()); 
+      }]
+      >,
+      InterfaceMethod<"",
       "bool", "isDeclarationForLinker", (ins), [{}],
       /*defaultImplementation=*/[{
         if ($_op.hasAvailableExternallyLinkage())
@@ -66,14 +72,21 @@ let cppNamespace = "::mlir::cir" in {
       }]
       >,
       InterfaceMethod<"",
+      "bool", "hasComdat", (ins), [{}],
+      /*defaultImplementation=*/[{
+        return $_op.getComdat();
+      }]
+      >,
+      InterfaceMethod<"",
       "void", "setDSOLocal", (ins "bool":$val), [{}],
       /*defaultImplementation=*/[{
         $_op.setDsolocal(val);
       }]
       >,
     ];
     let extraClassDeclaration = [{ 
-    bool canBenefitFromLocalAlias() const;
+    bool hasDefaultVisibility();  
+    bool canBenefitFromLocalAlias();
   }];
   }
 

clang/lib/CIR/CodeGen/CIRGenModule.cpp

@@ -365,22 +365,12 @@ bool CIRGenModule::MayBeEmittedEagerly(const ValueDecl *Global) {
   return true;
 }
 
-static bool hasDefaultVisibility(CIRGlobalValueInterface GV) {
-  // Since we do not support hidden visibility and private visibility,
-  // we can assume that the default visibility is public or private.
-  // The way we use private visibility now simply is just treating it
-  // as either local or private linkage, or just default for declarations
-  assert(!MissingFeatures::hiddenVisibility());
-  assert(!MissingFeatures::protectedVisibility());
-  return GV.isPublic() || GV.isPrivate();
-}
-
 static bool shouldAssumeDSOLocal(const CIRGenModule &CGM,
                                  CIRGlobalValueInterface GV) {
   if (GV.hasLocalLinkage())
     return true;
 
-  if (!hasDefaultVisibility(GV) && !GV.hasExternalWeakLinkage()) {
+  if (!GV.hasDefaultVisibility() && !GV.hasExternalWeakLinkage()) {
     return true;
   }
 
@@ -1436,8 +1426,11 @@ generateStringLiteral(mlir::Location loc, mlir::TypedAttr C,
   // TODO(cir)
   assert(!cir::MissingFeatures::threadLocal() && "NYI");
   assert(!cir::MissingFeatures::unnamedAddr() && "NYI");
-  assert(!mlir::cir::isWeakForLinker(LT) && "NYI");
-  assert(!cir::MissingFeatures::setDSOLocal() && "NYI");
+  if (GV.isWeakForLinker()) {
+    assert(CGM.supportsCOMDAT() && "Only COFF uses weak string literals");
+    GV.setComdat(true);
+  }
+  CGM.setDSOLocal(static_cast<mlir::Operation *>(GV));
   return GV;
 }
 
@@ -2910,10 +2903,10 @@ mlir::cir::GlobalOp CIRGenModule::createOrReplaceCXXRuntimeVariable(
     OldGV->erase();
   }
 
-  assert(!MissingFeatures::setComdat());
   if (supportsCOMDAT() && mlir::cir::isWeakForLinker(Linkage) &&
-      !GV.hasAvailableExternallyLinkage())
-    assert(!MissingFeatures::setComdat());
+      !GV.hasAvailableExternallyLinkage()) {
+    GV.setComdat(true);
+  }
 
   GV.setAlignmentAttr(getSize(Alignment));
   setDSOLocal(static_cast<mlir::Operation *>(GV));

clang/lib/CIR/Dialect/IR/CIRDialect.cpp

@@ -2109,16 +2109,16 @@ void cir::FuncOp::print(OpAsmPrinter &p) {
   if (getNoProto())
     p << "no_proto ";
 
+  if (getComdat())
+    p << "comdat ";
+
   if (getLinkage() != GlobalLinkageKind::ExternalLinkage)
     p << stringifyGlobalLinkageKind(getLinkage()) << ' ';
 
   auto vis = getVisibility();
   if (vis != mlir::SymbolTable::Visibility::Public)
     p << vis << " ";
 
-  if (getDsolocal())
-    p << "dsolocal ";
-
   // Print function name, signature, and control.
   p.printSymbolName(getSymName());
   auto fnType = getFunctionType();
@@ -2148,6 +2148,7 @@ void cir::FuncOp::print(OpAsmPrinter &p) {
           getSymVisibilityAttrName(),
           getArgAttrsAttrName(),
           getResAttrsAttrName(),
+          getComdatAttrName(),
       });
 
   if (auto aliaseeName = getAliasee()) {

clang/lib/CIR/Interfaces/CIROpInterfaces.cpp

@@ -17,8 +17,21 @@ using namespace mlir::cir;
 
 #include "clang/CIR/MissingFeatures.h"
 
-bool CIRGlobalValueInterface::canBenefitFromLocalAlias() const {
+bool CIRGlobalValueInterface::hasDefaultVisibility() {
+  assert(!::cir::MissingFeatures::hiddenVisibility());
+  assert(!::cir::MissingFeatures::protectedVisibility());
+  return isPublic() || isPrivate();
+}
+
+bool CIRGlobalValueInterface::canBenefitFromLocalAlias() {
   assert(!::cir::MissingFeatures::supportIFuncAttr());
-  assert(!::cir::MissingFeatures::setComdat());
+  // hasComdat here should be isDeduplicateComdat, but as far as clang codegen
+  // is concerned, there is no case for Comdat::NoDeduplicate as all comdat
+  // would be Comdat::Any or Comdat::Largest (in the case of MS ABI). And CIRGen
+  // wouldn't even generate Comdat::Largest comdat as it tries to leave ABI
+  // specifics to LLVM lowering stage, thus here we don't need test Comdat
+  // selectionKind.
+  return hasDefaultVisibility() && isExternalLinkage() && !isDeclaration() &&
+         !hasComdat();
   return false;
 }

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

@@ -46,12 +46,12 @@
 #include "mlir/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.h"
 #include "mlir/Target/LLVMIR/Export.h"
 #include "mlir/Transforms/DialectConversion.h"
-#include "clang/CIR/LoweringHelpers.h"
 #include "clang/CIR/Dialect/IR/CIRAttrs.h"
 #include "clang/CIR/Dialect/IR/CIRDialect.h"
 #include "clang/CIR/Dialect/IR/CIROpsEnums.h"
 #include "clang/CIR/Dialect/IR/CIRTypes.h"
 #include "clang/CIR/Dialect/Passes.h"
+#include "clang/CIR/LoweringHelpers.h"
 #include "clang/CIR/MissingFeatures.h"
 #include "clang/CIR/Passes.h"
 #include "llvm/ADT/APInt.h"
@@ -1433,7 +1433,7 @@ class CIRFuncLowering : public mlir::OpConversionPattern<mlir::cir::FuncOp> {
         cast<mlir::cir::ExtraFuncAttributesAttr>(extraAttrsEntry.getValue());
     if (!oldExtraAttrs.getElements().contains(attrKey))
       return;
-    
+
     mlir::NamedAttrList newExtraAttrs;
     for (auto entry : oldExtraAttrs.getElements()) {
       if (entry.getName() == attrKey) {
@@ -1815,13 +1815,35 @@ class CIRGlobalOpLowering
     }
 
     // Rewrite op.
-    rewriter.replaceOpWithNewOp<mlir::LLVM::GlobalOp>(
+    auto llvmGlobalOp = rewriter.replaceOpWithNewOp<mlir::LLVM::GlobalOp>(
         op, llvmType, isConst, linkage, symbol, init.value(),
         /*alignment*/ 0, /*addrSpace*/ 0,
         /*dsoLocal*/ false, /*threadLocal*/ (bool)op.getTlsModelAttr(),
         /*comdat*/ mlir::SymbolRefAttr(), attributes);
+    auto mod = op->getParentOfType<mlir::ModuleOp>();
+    if (op.getComdat())
+      addComdat(llvmGlobalOp, comdatOp, rewriter, mod);
     return mlir::success();
   }
+
+private:
+  mutable mlir::LLVM::ComdatOp comdatOp = nullptr;
+  static void addComdat(mlir::LLVM::GlobalOp &op,
+                        mlir::LLVM::ComdatOp &comdatOp,
+                        mlir::OpBuilder &builder, mlir::ModuleOp &module) {
+    StringRef comdatName("__llvm_comdat_globals");
+    if (!comdatOp) {
+      builder.setInsertionPointToStart(module.getBody());
+      comdatOp =
+          builder.create<mlir::LLVM::ComdatOp>(module.getLoc(), comdatName);
+    }
+    builder.setInsertionPointToStart(&comdatOp.getBody().back());
+    auto selectorOp = builder.create<mlir::LLVM::ComdatSelectorOp>(
+        comdatOp.getLoc(), op.getSymName(), mlir::LLVM::comdat::Comdat::Any);
+    op.setComdatAttr(mlir::SymbolRefAttr::get(
+        builder.getContext(), comdatName,
+        mlir::FlatSymbolRefAttr::get(selectorOp.getSymNameAttr())));
+  }
 };
 
 class CIRUnaryOpLowering
@@ -3415,7 +3437,7 @@ class CIRUndefOpLowering
   matchAndRewrite(mlir::cir::UndefOp op, OpAdaptor adaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
     auto typ = getTypeConverter()->convertType(op.getRes().getType());
-    
+
     rewriter.replaceOpWithNewOp<mlir::LLVM::UndefOp>(op, typ);
     return mlir::success();
   }
@@ -3455,7 +3477,8 @@ void populateCIRToLLVMConversionPatterns(mlir::RewritePatternSet &patterns,
       CIRRintOpLowering, CIRRoundOpLowering, CIRSinOpLowering,
       CIRSqrtOpLowering, CIRTruncOpLowering, CIRCopysignOpLowering,
       CIRFModOpLowering, CIRFMaxOpLowering, CIRFMinOpLowering, CIRPowOpLowering,
-      CIRClearCacheOpLowering, CIRUndefOpLowering>(converter, patterns.getContext());
+      CIRClearCacheOpLowering, CIRUndefOpLowering>(converter,
+                                                   patterns.getContext());
 }
 
 namespace {

clang/test/CIR/CodeGen/address-space-conversion.cpp

@@ -10,7 +10,7 @@ using ri1_t = int __attribute__((address_space(1))) &;
 using ri2_t = int __attribute__((address_space(2))) &;
 
 // CIR: cir.func @{{.*test_ptr.*}}
-// LLVM: define void @{{.*test_ptr.*}}
+// LLVM: define dso_local void @{{.*test_ptr.*}}
 void test_ptr() {
   pi1_t ptr1;
   pi2_t ptr2 = (pi2_t)ptr1;
@@ -24,7 +24,7 @@ void test_ptr() {
 }
 
 // CIR: cir.func @{{.*test_ref.*}}
-// LLVM: define void @{{.*test_ref.*}}
+// LLVM: define dso_local void @{{.*test_ref.*}}
 void test_ref() {
   pi1_t ptr;
   ri1_t ref1 = *ptr;
@@ -43,7 +43,7 @@ void test_ref() {
 }
 
 // CIR: cir.func @{{.*test_nullptr.*}}
-// LLVM: define void @{{.*test_nullptr.*}}
+// LLVM: define dso_local void @{{.*test_nullptr.*}}
 void test_nullptr() {
   constexpr pi1_t null1 = nullptr;
   pi2_t ptr = (pi2_t)null1;

clang/test/CIR/CodeGen/address-space.c

@@ -4,19 +4,19 @@
 // RUN: FileCheck --input-file=%t.ll %s -check-prefix=LLVM
 
 // CIR: cir.func {{@.*foo.*}}(%arg0: !cir.ptr<!s32i, addrspace(target<1>)>
-// LLVM: define void @foo(ptr addrspace(1) %0)
+// LLVM: define dso_local void @foo(ptr addrspace(1) %0)
 void foo(int __attribute__((address_space(1))) *arg) {
   return;
 }
 
 // CIR: cir.func {{@.*bar.*}}(%arg0: !cir.ptr<!s32i, addrspace(target<0>)>
-// LLVM: define void @bar(ptr %0)
+// LLVM: define dso_local void @bar(ptr %0)
 void bar(int __attribute__((address_space(0))) *arg) {
   return;
 }
 
 // CIR: cir.func {{@.*baz.*}}(%arg0: !cir.ptr<!s32i>
-// LLVM: define void @baz(ptr %0)
+// LLVM: define dso_local void @baz(ptr %0)
 void baz(int *arg) {
   return;
 }

clang/test/CIR/CodeGen/atomic.cpp

@@ -28,7 +28,7 @@ int basic_binop_fetch(int *i) {
 // CHECK:  %[[VAL:.*]] = cir.load %[[ONE_ADDR]] : !cir.ptr<!s32i>, !s32i
 // CHECK:  cir.atomic.fetch(add, %[[I]] : !cir.ptr<!s32i>, %[[VAL]] : !s32i, seq_cst) : !s32i
 
-// LLVM: define i32 @_Z17basic_binop_fetchPi
+// LLVM: define dso_local i32 @_Z17basic_binop_fetchPi
 // LLVM: %[[RMW:.*]] = atomicrmw add ptr {{.*}}, i32 %[[VAL:.*]] seq_cst, align 4
 // LLVM: add i32 %[[RMW]], %[[VAL]]
 
@@ -45,7 +45,7 @@ int other_binop_fetch(int *i) {
 // CHECK: cir.atomic.fetch(or, {{.*}}, acquire
 // CHECK: cir.atomic.fetch(xor, {{.*}}, release
 
-// LLVM: define i32 @_Z17other_binop_fetchPi
+// LLVM: define dso_local i32 @_Z17other_binop_fetchPi
 // LLVM: %[[RMW_SUB:.*]] = atomicrmw sub ptr {{.*}} monotonic
 // LLVM: sub i32 %[[RMW_SUB]], {{.*}}
 // LLVM: %[[RMW_AND:.*]] = atomicrmw and ptr {{.*}} acquire
@@ -62,7 +62,7 @@ int nand_binop_fetch(int *i) {
 // CHECK: cir.func @_Z16nand_binop_fetchPi
 // CHECK: cir.atomic.fetch(nand, {{.*}}, acq_rel
 
-// LLVM: define i32 @_Z16nand_binop_fetchPi
+// LLVM: define dso_local i32 @_Z16nand_binop_fetchPi
 // LLVM: %[[RMW_NAND:.*]] = atomicrmw nand ptr {{.*}} acq_rel
 // LLVM: %[[AND:.*]] = and i32 %[[RMW_NAND]]
 // LLVM: = xor i32 %[[AND]], -1
@@ -76,7 +76,7 @@ int fp_binop_fetch(float *i) {
 // CHECK: cir.atomic.fetch(add,
 // CHECK: cir.atomic.fetch(sub,
 
-// LLVM: define i32 @_Z14fp_binop_fetchPf
+// LLVM: define dso_local i32 @_Z14fp_binop_fetchPf
 // LLVM: %[[RMW_FADD:.*]] = atomicrmw fadd ptr
 // LLVM: fadd float %[[RMW_FADD]]
 // LLVM: %[[RMW_FSUB:.*]] = atomicrmw fsub ptr
@@ -99,7 +99,7 @@ int fetch_binop(int *i) {
 // CHECK: cir.atomic.fetch(xor, {{.*}}) fetch_first
 // CHECK: cir.atomic.fetch(nand, {{.*}}) fetch_first
 
-// LLVM: define i32 @_Z11fetch_binopPi
+// LLVM: define dso_local i32 @_Z11fetch_binopPi
 // LLVM: atomicrmw add ptr
 // LLVM-NOT: add {{.*}}
 // LLVM: atomicrmw sub ptr
@@ -126,7 +126,7 @@ void min_max_fetch(int *i) {
 // CHECK: = cir.atomic.fetch(max, {{.*}}) : !s32i
 // CHECK: = cir.atomic.fetch(min, {{.*}}) : !s32i
 
-// LLVM: define void @_Z13min_max_fetchPi
+// LLVM: define dso_local void @_Z13min_max_fetchPi
 // LLVM: atomicrmw max ptr
 // LLVM-NOT: icmp {{.*}}
 // LLVM: atomicrmw min ptr
@@ -343,7 +343,7 @@ void inc_int(int* a, int b) {
   int c = __sync_fetch_and_add(a, b);
 }
 // CHECK-LABEL: @_Z7inc_int
-// CHECK: %[[PTR:.*]] = cir.load {{.*}} : !cir.ptr<!cir.ptr<!s32i>>, !cir.ptr<!s32i> 
+// CHECK: %[[PTR:.*]] = cir.load {{.*}} : !cir.ptr<!cir.ptr<!s32i>>, !cir.ptr<!s32i>
 // CHECK: %[[VAL:.*]] = cir.load {{.*}} : !cir.ptr<!s32i>, !s32i
 // CHECK: %[[RES:.*]] = cir.atomic.fetch(add, %[[PTR]] : !cir.ptr<!s32i>, %[[VAL]] : !s32i, seq_cst) fetch_first : !s32i
 // CHECK: cir.store %[[RES]], {{.*}} : !s32i, !cir.ptr<!s32i>
@@ -442,7 +442,7 @@ void cmp_bool_byte(char* p, char x, char u) {
 // LLVM: %[[TMP:.*]] = extractvalue { i32, i1 } %[[RES]], 0
 // LLVM: store i32 %[[TMP]], ptr {{.*}}
 void cmp_val_int(int* p, int x, int u) {
-  int r = __sync_val_compare_and_swap(p, x, u);  
+  int r = __sync_val_compare_and_swap(p, x, u);
 }
 
 // CHECK-LABEL: @_Z12cmp_val_long
@@ -466,7 +466,7 @@ void cmp_val_short(short* p, short x, short u) {
 // CHECK-LABEL: @_Z12cmp_val_byte
 // CHECK: cir.atomic.cmp_xchg({{.*}} : !cir.ptr<!s8i>, {{.*}} : !s8i, {{.*}} : !s8i, success = seq_cst, failure = seq_cst) : (!s8i, !cir.bool)
 
-// LLVM-LABEL: @_Z12cmp_val_byte 
+// LLVM-LABEL: @_Z12cmp_val_byte
 // LLVM: cmpxchg ptr {{.*}}, i8 {{.*}}, i8 {{.*}} seq_cst seq_cst
 void cmp_val_byte(char* p, char x, char u) {
   char r = __sync_val_compare_and_swap(p, x, u);

clang/test/CIR/CodeGen/basic.c

@@ -30,7 +30,7 @@ int f2(void) { return 3; }
 // CIR-NEXT: %2 = cir.load %0 : !cir.ptr<!s32i>, !s32i
 // CIR-NEXT: cir.return %2 : !s32i
 
-// LLVM: define i32 @f2()
+// LLVM: define dso_local i32 @f2()
 // LLVM-NEXT:  %1 = alloca i32, i64 1, align 4
 // LLVM-NEXT:  store i32 3, ptr %1, align 4
 // LLVM-NEXT:  %2 = load i32, ptr %1, align 4