[mlir][FuncToLLVM] Drop llvm.linkage attribute

[Dinistro]

Summary

This commit ensures that all functions produced by FuncToLLVM drop the
llvm.linkage attribute. Furthermore, it adds a small test that checks if
the readnone attribute is preserved.

[Dinistro]

Small comment change

[gysit]

mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp

67

ultra nit: Drop -> Drops.

I am not sure about the second half of the comment ("or are attached to the function manually". In general the function seems to copy all manually set attributes except for linkageAttrName? Shouldn't we just drop this second half of the sentence?

mlir/test/Conversion/FuncToLLVM/emit-c-wrappers-for-external-callers.mlir

76

nit: what about calling the function "drop_default_linkage"? Then it is more obvious why the linkage is not printed?

[Dinistro]

Addressed review comments

[Dinistro]

mlir/test/Conversion/FuncToLLVM/emit-c-wrappers-for-external-callers.mlir

76

This does not just drop the external linkage but all linkage attributes. I changed the other test to use weak linkage to demonstrate that this influences the signature but no longer appears as an attribute.

mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp

 #define GEN_PASS_DEF_CONVERTFUNCTOLLVM
 #include "mlir/Conversion/Passes.h.inc"
 } // namespace mlir
 
 using namespace mlir;
 
 #define PASS_NAME "convert-func-to-llvm"
 
+static constexpr StringRef varargsAttrName = "func.varargs";
+static constexpr StringRef linkageAttrName = "llvm.linkage";
+
 /// Only retain those attributes that are not constructed by
 /// `LLVMFuncOp::build`. If `filterArgAttrs` is set, also filter out argument
 /// attributes.
 static void filterFuncAttributes(func::FuncOp func, bool filterArgAndResAttrs,
                                  SmallVectorImpl<NamedAttribute> &result) {
   for (const NamedAttribute &attr : func->getAttrs()) {
     if (attr.getName() == SymbolTable::getSymbolAttrName() ||
         attr.getName() == func.getFunctionTypeAttrName() ||
-        attr.getName() == "func.varargs" ||
+        attr.getName() == linkageAttrName ||
+        attr.getName() == varargsAttrName ||
         (filterArgAndResAttrs &&
          (attr.getName() == func.getArgAttrsAttrName() ||
           attr.getName() == func.getResAttrsAttrName())))
       continue;
     result.push_back(attr);
   }
 }
 
protected:
 
   // Convert input FuncOp to LLVMFuncOp by using the LLVMTypeConverter provided
   // to this legalization pattern.
   LLVM::LLVMFuncOp
   convertFuncOpToLLVMFuncOp(func::FuncOp funcOp,
                             ConversionPatternRewriter &rewriter) const {
     // Convert the original function arguments. They are converted using the
     // LLVMTypeConverter provided to this legalization pattern.
-    auto varargsAttr = funcOp->getAttrOfType<BoolAttr>("func.varargs");
+    auto varargsAttr = funcOp->getAttrOfType<BoolAttr>(varargsAttrName);
     TypeConverter::SignatureConversion result(funcOp.getNumArguments());
     auto llvmType = getTypeConverter()->convertFunctionSignature(
         funcOp.getFunctionType(), varargsAttr && varargsAttr.getValue(),
         result);
     if (!llvmType)
       return nullptr;
 
     // Propagate argument/result attributes to all converted arguments/result
if (ArrayAttr argAttrDicts = funcOp.getAllArgAttrs()) {
         assert(mapping && "unexpected deletion of function argument");
         for (size_t j = 0; j < mapping->size; ++j)
           newArgAttrs[mapping->inputNo + j] =
               DictionaryAttr::get(rewriter.getContext(), convertedAttrs);
       }
       attributes.push_back(rewriter.getNamedAttr(
           funcOp.getArgAttrsAttrName(), rewriter.getArrayAttr(newArgAttrs)));
     }
-    for (const auto &pair : llvm::enumerate(attributes)) {
-      if (pair.value().getName() == "llvm.linkage") {
-        attributes.erase(attributes.begin() + pair.index());
-        break;
-      }
-    }
 
     // Create an LLVM function, use external linkage by default until MLIR
     // functions have linkage.
     LLVM::Linkage linkage = LLVM::Linkage::External;
-    if (funcOp->hasAttr("llvm.linkage")) {
+    if (funcOp->hasAttr(linkageAttrName)) {
       auto attr =
-          funcOp->getAttr("llvm.linkage").dyn_cast<mlir::LLVM::LinkageAttr>();
+          funcOp->getAttr(linkageAttrName).dyn_cast<mlir::LLVM::LinkageAttr>();
       if (!attr) {
-        funcOp->emitError()
-            << "Contains llvm.linkage attribute not of type LLVM::LinkageAttr";
+        funcOp->emitError() << "Contains " << linkageAttrName
+                            << " attribute not of type LLVM::LinkageAttr";
         return nullptr;
       }
       linkage = attr.getLinkage();
     }
     auto newFuncOp = rewriter.create<LLVM::LLVMFuncOp>(
         funcOp.getLoc(), funcOp.getName(), llvmType, linkage,
         /*dsoLocal*/ false, /*cconv*/ LLVM::CConv::C, attributes);
     rewriter.inlineRegionBefore(funcOp.getBody(), newFuncOp.getBody(),

mlir/test/Conversion/FuncToLLVM/convert-funcs.mlir

 ^bb1(%bbarg: () -> ()):
 // CHECK-NEXT:  llvm.return %0 : !llvm.ptr<func<void ()>>
   return %bbarg : () -> ()
 }
 
 // CHECK-LABEL: llvm.func extern_weak @llvmlinkage(i32)
 func.func private @llvmlinkage(i32) attributes { "llvm.linkage" = #llvm.linkage<extern_weak> }
 
+// CHECK-LABEL: llvm.func @llvmreadnone(i32)
+// CHECK-SAME: llvm.readnone
+func.func private @llvmreadnone(i32) attributes { llvm.readnone }
+
 // CHECK-LABEL: llvm.func @body(i32)
 func.func private @body(i32)
 
 // CHECK-LABEL: llvm.func @indirect_const_call
 // CHECK-SAME: (%[[ARG0:.*]]: i32) {
 func.func @indirect_const_call(%arg0: i32) {
 // CHECK-NEXT: %[[ADDR:.*]] = llvm.mlir.addressof @body : !llvm.ptr<func<void (i32)>>
   %0 = constant @body : (i32) -> ()

mlir/test/Conversion/FuncToLLVM/emit-c-wrappers-for-external-callers.mlir

 // CHECK-LABEL: llvm.func @_mlir_ciface_multiple_arg_attr_multiple_res_attr
 // CHECK: (%{{.*}}: !llvm.ptr<{{.*}}> {llvm.struct_attrs = [{}, {test.returnOne = 1 : i64}, {test.returnTwo = 2 : i64}]}, %{{.*}}: !llvm.ptr<{{.*}}> {test.argZero = 0 : i64}, %{{.*}}: f32, %{{.*}}: i32 {test.argTwo = 2 : i64}
 func.func @multiple_arg_attr_multiple_res_attr(%arg0: memref<f32> {test.argZero = 0}, %arg1: f32, %arg2: i32 {test.argTwo = 2}) -> (f32, memref<i32> {test.returnOne = 1}, i32 {test.returnTwo = 2}) {
   %0 = arith.constant 1.00 : f32
   %1 = memref.alloc() : memref<i32>
   %2 = arith.constant 2 : i32
   return %0, %1, %2 : f32, memref<i32>, i32
 }
+
+// CHECK: llvm.func @drop_linkage_attr() -> (!llvm.struct{{.*}} {test.returnOne})
+// CHECK-LABEL: llvm.func @_mlir_ciface_drop_linkage_attr
+// CHECK-NOT: llvm.linkage
+// CHECK: %{{.*}}: !llvm.ptr{{.*}} {test.returnOne}
+func.func @drop_linkage_attr() -> (memref<f32> {test.returnOne}) attributes { llvm.linkage = #llvm.linkage<external> } {
+  %0 = memref.alloc() : memref<f32>
+  return %0 : memref<f32>
+}

mlir/test/Conversion/FuncToLLVM/emit-c-wrappers-for-external-functions.mlir

 // CHECK-LABEL: llvm.func @_mlir_ciface_one_arg_attr_one_res_attr_with_value_return
 // CHECK: (!llvm.ptr<{{.*}}> {test.argOne = 1 : i64}) -> (f32 {test.returnOne = 1 : i64})
 func.func private @one_arg_attr_one_res_attr_with_value_return(%arg0: memref<f32> {test.argOne = 1}) -> (f32 {test.returnOne = 1})
 
 // CHECK: llvm.func @multiple_arg_attr_multiple_res_attr({{.*}}) -> (!llvm.struct<{{.*}}> {llvm.struct_attrs = [{}, {test.returnOne = 1 : i64}, {test.returnTwo = 2 : i64}]})
 // CHECK-LABEL: llvm.func @_mlir_ciface_multiple_arg_attr_multiple_res_attr
 // CHECK: (!llvm.ptr<{{.*}}> {llvm.struct_attrs = [{}, {test.returnOne = 1 : i64}, {test.returnTwo = 2 : i64}]}, !llvm.ptr<{{.*}}> {test.argZero = 0 : i64}, f32, i32 {test.argTwo = 2 : i64}
 func.func private @multiple_arg_attr_multiple_res_attr(%arg0: memref<f32> {test.argZero = 0}, %arg1: f32, %arg2: i32 {test.argTwo = 2}) -> (f32, memref<i32> {test.returnOne = 1}, i32 {test.returnTwo = 2})
+
+// CHECK: llvm.func weak @drop_linkage_attr() -> (!llvm.struct{{.*}} {test.returnOne})
+// CHECK-LABEL: llvm.func @_mlir_ciface_drop_linkage_attr
+// CHECK-NOT: llvm.linkage
+// CHECK: !llvm.ptr{{.*}} {test.returnOne}
+func.func private @drop_linkage_attr() -> (memref<f32> {test.returnOne}) attributes { llvm.linkage = #llvm.linkage<weak> }