Make LLVM Linkage a first class attribute instead of using an integer attribute

[mehdi_amini]

Summary

This makes the IR more readable, in particular when this will be used on
the builtin func outside of the LLVM dialect.

[mehdi_amini]

Move using linkage::getMaxEnumValForLinkage; to the implementation file

[wsmoses]

Yeah this is a lot nicer (as clearly evidenced by the llvm.mlir globals as well). For my own edification, is there documentation about the tablegen storageType/convertFromStorage parameters?

[rriddle]

mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td

34

Can you drop this.

[mehdi_amini]

In D109209#2982980, @wsmoses wrote:

Yeah this is a lot nicer (as clearly evidenced by the llvm.mlir globals as well). For my own edification, is there documentation about the tablegen storageType/convertFromStorage parameters?

Unfortunately not, the best may be: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/OpBase.td#L823-L836
But it also impact the code generated for the builder of the operation using these attributes. For example the fact that storageType and returnType don't match here:

let storageType = "::mlir::LLVM::LinkageAttr";
let returnType = "::mlir::LLVM::Linkage";

means that when an operation declares a Linkage attribute in its argument list, an extra builder will be generated that takes directly an mlir::LLVM::Linkage enum value (but only because we also define let constBuilderCall = "::mlir::LLVM::LinkageAttr::get($_builder.getContext(), $0)"; which indicates how to build the attribute from the enum value...).

[mehdi_amini]

Remove the empty extraClassDeclaration

mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td

 // All of the attributes will extend this class.
 class LLVM_Attr<string name> : AttrDef<LLVM_Dialect, name>;
 
 // The "FastMath" flags associated with floating point LLVM instructions.
 def FastmathFlagsAttr : LLVM_Attr<"FMF"> {
   let mnemonic = "fastmath";
 
   // List of type parameters.
-  let parameters = (
-    ins
+  let parameters = (ins
     "FastmathFlags":$flags
   );
 }
 
+// Attribute definition for the LLVM Linkage enum.
+def LinkageAttr : LLVM_Attr<"Linkage"> {
+  let mnemonic = "linkage";
+  let parameters = (ins
+    "linkage::Linkage":$linkage
+  );
+  let extraClassDeclaration = [{
+  }];
+}
+
 def LoopOptionsAttr : LLVM_Attr<"LoopOptions"> {
   let mnemonic = "loopopts";
 
   let description = [{
     This attributes encapsulates "loop options". It is means to decorate
     branches that are "latches" (loop backedges) and maps to the `!llvm.loop`
     metadatas: https://llvm.org/docs/LangRef.html#llvm-loop
     It store the options as a pair <enum,int64_t> in a sorted array and expose
     APIs to retrieve the value for each option with a stronger type (bool for
     example).
   }];
 
   // List of type parameters.
-  let parameters = (
-    ins
+  let parameters = (ins
     ArrayRefParameter<"std::pair<LoopOptionCase, int64_t>", "">:$options
   );
 
   let extraClassDeclaration = [{
     using OptionValuePair = std::pair<LoopOptionCase, int64_t>;
     using OptionsArray = ArrayRef<std::pair<LoopOptionCase, int64_t>>;
     Optional<bool> disableUnroll();
     Optional<bool> disableLICM();

mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h

 #include "mlir/Interfaces/ControlFlowInterfaces.h"
 #include "mlir/Interfaces/SideEffectInterfaces.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
 
 #include "mlir/Dialect/LLVMIR/LLVMOpsEnums.h.inc"
+
+namespace mlir {
+namespace LLVM {
+// Inline the LLVM generated Linkage enum and utility.
+// This is only necessary to isolate the "enum generated code" from the
+// attribute definition itself.
+// TODO: this shouldn't be needed after we unify the attribute generation, i.e.
+// --gen-attr-* and --gen-attrdef-*.
+using linkage::getMaxEnumValForLinkage;
+using linkage::Linkage;
+} // namespace LLVM
+} // namespace mlir
+
 #include "mlir/Dialect/LLVMIR/LLVMOpsInterfaces.h.inc"
 
 namespace llvm {
 class Type;
 class LLVMContext;
 namespace sys {
 template <bool mt_only>
 class SmartMutex;

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

 def FMFninf     : BitEnumAttrCase<"ninf", 0x2>;
 def FMFnsz      : BitEnumAttrCase<"nsz", 0x4>;
 def FMFarcp     : BitEnumAttrCase<"arcp", 0x8>;
 def FMFcontract : BitEnumAttrCase<"contract", 0x10>;
 def FMFafn      : BitEnumAttrCase<"afn", 0x20>;
 def FMFreassoc  : BitEnumAttrCase<"reassoc", 0x40>;
 def FMFfast     : BitEnumAttrCase<"fast", 0x80>;
 
-def FastmathFlags : BitEnumAttr<
+def FastmathFlags_DoNotUse : BitEnumAttr<
     "FastmathFlags",
     "LLVM fastmath flags",
     [FMFnnan, FMFninf, FMFnsz, FMFarcp, FMFcontract, FMFafn, FMFreassoc, FMFfast
     ]> {
   let cppNamespace = "::mlir::LLVM";
 }
 
 def LLVM_FMFAttr : DialectAttr<
def LinkageExternWeak
    : LLVM_EnumAttrCase<"ExternWeak", "extern_weak", "ExternalWeakLinkage", 7>;
 def LinkageLinkonceODR
     : LLVM_EnumAttrCase<"LinkonceODR", "linkonce_odr", "LinkOnceODRLinkage", 8>;
 def LinkageWeakODR
     : LLVM_EnumAttrCase<"WeakODR", "weak_odr", "WeakODRLinkage", 9>;
 def LinkageExternal
     : LLVM_EnumAttrCase<"External", "external", "ExternalLinkage", 10>;
 
-def Linkage : LLVM_EnumAttr<
+def LinkageEnum : LLVM_EnumAttr<
     "Linkage",
     "::llvm::GlobalValue::LinkageTypes",
     "LLVM linkage types",
     [LinkagePrivate, LinkageInternal, LinkageAvailableExternally,
      LinkageLinkonce, LinkageWeak, LinkageCommon, LinkageAppending,
      LinkageExternWeak, LinkageLinkonceODR, LinkageWeakODR, LinkageExternal]> {
-  let cppNamespace = "::mlir::LLVM";
+  let cppNamespace = "::mlir::LLVM::linkage";
+}
+
+def Linkage : DialectAttr<
+    LLVM_Dialect,
+    CPred<"$_self.isa<::mlir::LLVM::LinkageAttr>()">,
+    "LLVM Linkage specification"> {
+  let storageType = "::mlir::LLVM::LinkageAttr";
+  let returnType = "::mlir::LLVM::Linkage";
+  let convertFromStorage = "$_self.getLinkage()";
+  let constBuilderCall =
+          "::mlir::LLVM::LinkageAttr::get($_builder.getContext(), $0)";
 }
 
+
 def UnnamedAddrNone : LLVM_EnumAttrCase<"None", "", "None", 0>;
 def UnnamedAddrLocal : LLVM_EnumAttrCase<"Local", "local_unnamed_addr", "Local", 1>;
 def UnnamedAddrGlobal : LLVM_EnumAttrCase<"Global", "unnamed_addr", "Global", 2>;
 
 def UnnamedAddr : LLVM_EnumAttr<
     "UnnamedAddr",
     "::llvm::GlobalValue::UnnamedAddr",
     "LLVM GlobalValue UnnamedAddr",

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

void GlobalOp::build(OpBuilder &builder, OperationState &result, Type type,
 
   // Only add an alignment attribute if the "alignment" input
   // is different from 0. The value must also be a power of two, but
   // this is tested in GlobalOp::verify, not here.
   if (alignment != 0)
     result.addAttribute("alignment", builder.getI64IntegerAttr(alignment));
 
   result.addAttribute(getLinkageAttrName(),
-                      builder.getI64IntegerAttr(static_cast<int64_t>(linkage)));
+                      LinkageAttr::get(builder.getContext(), linkage));
   if (addrSpace != 0)
     result.addAttribute("addr_space", builder.getI32IntegerAttr(addrSpace));
   result.attributes.append(attrs.begin(), attrs.end());
   result.addRegion();
 }
 
 static void printGlobalOp(OpAsmPrinter &p, GlobalOp op) {
   p << ' ' << stringifyLinkage(op.linkage()) << ' ';
struct EnumTraits<Ty> {                                                      \
     static StringRef stringify(Ty value) { return stringify##Ty(value); }      \
     static unsigned getMaxEnumVal() { return getMaxEnumValFor##Ty(); }         \
   }
 
 REGISTER_ENUM_TYPE(Linkage);
 REGISTER_ENUM_TYPE(UnnamedAddr);
 } // end namespace
 
-template <typename EnumTy>
-static ParseResult parseOptionalLLVMKeyword(OpAsmParser &parser,
-                                            OperationState &result,
-                                            StringRef name) {
+/// Parse an enum from the keyword, or default to the provided default value.
+/// The return type is the enum type by default, unless overriden with the
+/// second template argument.
+template <typename EnumTy, typename RetTy = EnumTy>
+static RetTy parseOptionalLLVMKeyword(OpAsmParser &parser,
+                                      EnumTy defaultValue) {
   SmallVector<StringRef, 10> names;
-  for (unsigned i = 0, e = getMaxEnumValForLinkage(); i <= e; ++i)
+  for (unsigned i = 0, e = EnumTraits<EnumTy>::getMaxEnumVal(); i <= e; ++i)
     names.push_back(EnumTraits<EnumTy>::stringify(static_cast<EnumTy>(i)));
 
   int index = parseOptionalKeywordAlternative(parser, names);
   if (index == -1)
-    return failure();
-  result.addAttribute(name, parser.getBuilder().getI64IntegerAttr(index));
-  return success();
+    return static_cast<RetTy>(defaultValue);
+  return static_cast<RetTy>(index);
 }
 
 // operation ::= `llvm.mlir.global` linkage? `constant`? `@` identifier
 //               `(` attribute? `)` align? attribute-list? (`:` type)? region?
 // align     ::= `align` `=` UINT64
 //
 // The type can be omitted for string attributes, in which case it will be
 // inferred from the value of the string as [strlen(value) x i8].
 static ParseResult parseGlobalOp(OpAsmParser &parser, OperationState &result) {
-  if (failed(parseOptionalLLVMKeyword<Linkage>(parser, result,
-                                               getLinkageAttrName())))
-    result.addAttribute(getLinkageAttrName(),
-                        parser.getBuilder().getI64IntegerAttr(
-                            static_cast<int64_t>(LLVM::Linkage::External)));
-
-  if (failed(parseOptionalLLVMKeyword<UnnamedAddr>(parser, result,
-                                                   getUnnamedAddrAttrName())))
-    result.addAttribute(getUnnamedAddrAttrName(),
-                        parser.getBuilder().getI64IntegerAttr(
-                            static_cast<int64_t>(LLVM::UnnamedAddr::None)));
+  MLIRContext *ctx = parser.getBuilder().getContext();
+  // Parse optional linkage, default to External.
+                      LLVM::LinkageAttr::get(
+                          ctx, parseOptionalLLVMKeyword<Linkage>(
+                                   parser, result, LLVM::Linkage::External)));
+  // Parse optional UnnamedAddr, default to None.
+                          parseOptionalLLVMKeyword<UnnamedAddr, int64_t>(
+                              parser, result, LLVM::UnnamedAddr::None)));
 
   if (succeeded(parser.parseOptionalKeyword("constant")))
     result.addAttribute("constant", parser.getBuilder().getUnitAttr());
 
   StringAttr name;
   if (parser.parseSymbolName(name, SymbolTable::getSymbolAttrName(),
                              result.attributes) ||
       parser.parseLParen())
void LLVMFuncOp::build(OpBuilder &builder, OperationState &result,
                        StringRef name, Type type, LLVM::Linkage linkage,
                        bool dsoLocal, ArrayRef<NamedAttribute> attrs,
                        ArrayRef<DictionaryAttr> argAttrs) {
   result.addRegion();
   result.addAttribute(SymbolTable::getSymbolAttrName(),
                       builder.getStringAttr(name));
   result.addAttribute("type", TypeAttr::get(type));
   result.addAttribute(getLinkageAttrName(),
-                      builder.getI64IntegerAttr(static_cast<int64_t>(linkage)));
+                      LinkageAttr::get(builder.getContext(), linkage));
   result.attributes.append(attrs.begin(), attrs.end());
   if (dsoLocal)
     result.addAttribute("dso_local", builder.getUnitAttr());
   if (argAttrs.empty())
     return;
 
   assert(type.cast<LLVMFunctionType>().getNumParams() == argAttrs.size() &&
          "expected as many argument attribute lists as arguments");

 // Parses an LLVM function.
 //
 // operation ::= `llvm.func` linkage? function-signature function-attributes?
 //               function-body
 //
 static ParseResult parseLLVMFuncOp(OpAsmParser &parser,
                                    OperationState &result) {
   // Default to external linkage if no keyword is provided.
-  if (failed(parseOptionalLLVMKeyword<Linkage>(parser, result,
-                                               getLinkageAttrName())))
-    result.addAttribute(getLinkageAttrName(),
-                        parser.getBuilder().getI64IntegerAttr(
-                            static_cast<int64_t>(LLVM::Linkage::External)));
+  result.addAttribute(
+      getLinkageAttrName(),
+      LinkageAttr::get(parser.getBuilder().getContext(),
+                       parseOptionalLLVMKeyword<Linkage>(
+                           parser, result, LLVM::Linkage::External)));
 
   StringAttr nameAttr;
   SmallVector<OpAsmParser::OperandType, 8> entryArgs;
   SmallVector<NamedAttrList, 1> argAttrs;
   SmallVector<NamedAttrList, 1> resultAttrs;
   SmallVector<Type, 8> argTypes;
   SmallVector<Type, 4> resultTypes;
   bool isVariadic;
static LogicalResult verify(FenceOp &op) {
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // LLVMDialect initialization, type parsing, and registration.
 //===----------------------------------------------------------------------===//
 
 void LLVMDialect::initialize() {
-  addAttributes<FMFAttr, LoopOptionsAttr>();
+  addAttributes<FMFAttr, LinkageAttr, LoopOptionsAttr>();
 
   // clang-format off
   addTypes<LLVMVoidType,
            LLVMPPCFP128Type,
            LLVMX86MMXType,
            LLVMTokenType,
            LLVMLabelType,
            LLVMMetadataType,
if (failed(parser.parseOptionalGreater())) {
 
     if (failed(parser.parseGreater()))
       return {};
   }
 
   return FMFAttr::get(parser.getBuilder().getContext(), flags);
 }
 
+void LinkageAttr::print(DialectAsmPrinter &printer) const {
+  printer << "linkage<";
+  if (static_cast<uint64_t>(getLinkage()) <= getMaxEnumValForLinkage())
+    printer << stringifyEnum(getLinkage());
+  else
+    printer << static_cast<uint64_t>(getLinkage());
+  printer << ">";
+}
+
+Attribute LinkageAttr::parse(MLIRContext *context, DialectAsmParser &parser,
+                             Type type) {
+  StringRef elemName;
+  if (parser.parseLess() || parser.parseKeyword(&elemName) ||
+      parser.parseGreater())
+    return {};
+  auto elem = linkage::symbolizeLinkage(elemName);
+  if (!elem) {
+    parser.emitError(parser.getNameLoc(), "Unknown linkage: ") << elemName;
+    return {};
+  }
+  Linkage linkage = *elem;
+  return LinkageAttr::get(context, linkage);
+}
+
 LoopOptionsAttrBuilder::LoopOptionsAttrBuilder(LoopOptionsAttr attr)
     : options(attr.getOptions().begin(), attr.getOptions().end()) {}
 
 template <typename T>
 LoopOptionsAttrBuilder &LoopOptionsAttrBuilder::setOption(LoopOptionCase tag,
                                                           Optional<T> value) {
   auto option = llvm::find_if(
       options, [tag](auto option) { return option.first == tag; });

mlir/test/Dialect/LLVMIR/func.mlir

module {
   llvm.func weak @weak_linkage() {
     llvm.return
   }
 
   // Omit the `external` linkage, which is the default, in the custom format.
   // Check that it is present in the generic format using its numeric value.
   //
   // CHECK: llvm.func @external_func
-  // GENERIC: linkage = 10
+  // GENERIC: linkage = #llvm.linkage<external>
   llvm.func external @external_func()
 }
 
 // -----
 
 module {
   // expected-error@+1 {{requires one region}}
   "llvm.func"() {sym_name = "no_region", type = !llvm.func<void ()>} : () -> ()

mlir/test/Dialect/LLVMIR/global.mlir

 // -----
 
 // expected-error @+1 {{expects type to be a valid element type for an LLVM pointer}}
 llvm.mlir.global internal constant @constant(37.0) : !llvm.label
 
 // -----
 
 // expected-error @+1 {{'addr_space' failed to satisfy constraint: 32-bit signless integer attribute whose value is non-negative}}
-"llvm.mlir.global"() ({}) {sym_name = "foo", type = i64, value = 42 : i64, addr_space = -1 : i32, linkage = 0} : () -> ()
+"llvm.mlir.global"() ({}) {sym_name = "foo", type = i64, value = 42 : i64, addr_space = -1 : i32, linkage = #llvm.linkage<private>} : () -> ()
 
 // -----
 
 // expected-error @+1 {{'addr_space' failed to satisfy constraint: 32-bit signless integer attribute whose value is non-negative}}
-"llvm.mlir.global"() ({}) {sym_name = "foo", type = i64, value = 42 : i64, addr_space = 1.0 : f32, linkage = 0} : () -> ()
+"llvm.mlir.global"() ({}) {sym_name = "foo", type = i64, value = 42 : i64, addr_space = 1.0 : f32, linkage = #llvm.linkage<private>} : () -> ()
 
 // -----
 
 func @foo() {
   // expected-error @+1 {{must appear at the module level}}
   llvm.mlir.global internal @bar(42) : i32
 
   return

mlir/test/Target/LLVMIR/llvmir.mlir

 // RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
 
 // CHECK: @global_aligned32 = private global i64 42, align 32
-"llvm.mlir.global"() ({}) {sym_name = "global_aligned32", type = i64, value = 42 : i64, linkage = 0, alignment = 32} : () -> ()
+"llvm.mlir.global"() ({}) {sym_name = "global_aligned32", type = i64, value = 42 : i64, linkage = #llvm.linkage<private>, alignment = 32} : () -> ()
 
 // CHECK: @global_aligned64 = private global i64 42, align 64
 llvm.mlir.global private @global_aligned64(42 : i64) {alignment = 64 : i64} : i64
 
 // CHECK: @global_aligned64_native = private global i64 42, align 64
 llvm.mlir.global private @global_aligned64_native(42 : i64) { alignment = 64 } : i64
 
 // CHECK: @i32_global = internal global i32 42