[Mlir-commits] [mlir] Update MLIR conversion to LLVMFunc to account better for properties (PR #67406)

Mehdi Amini llvmlistbot at llvm.org
Sun Oct 8 21:19:06 PDT 2023


https://github.com/joker-eph updated https://github.com/llvm/llvm-project/pull/67406

>From b26c0db8a64c588d19ad4318882eb9695da86396 Mon Sep 17 00:00:00 2001
From: Mehdi Amini <joker.eph at gmail.com>
Date: Tue, 26 Sep 2023 01:44:14 -0700
Subject: [PATCH] Update MLIR conversion to LLVMFunc to account better for
 properties

Change the attribute propagation to handle only discardable attributes,
inherent attribute are handled directly and args/results through the
interface.
---
 .../Fir/convert-to-llvm-openmp-and-fir.fir    |   4 +-
 flang/test/Fir/convert-to-llvm.fir            |   4 +-
 flang/test/Fir/external-mangling.fir          |   8 +-
 flang/test/Fir/rename-msvc-libm.fir           |   2 +-
 flang/test/Fir/tbaa.fir                       |   6 +-
 flang/test/Intrinsics/math-codegen.fir        |   4 +-
 .../Lower/intrinsic-procedure-wrappers.f90    |   2 +-
 mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td   |   1 +
 mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp | 175 ++++++++----------
 mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp    |  18 +-
 10 files changed, 110 insertions(+), 114 deletions(-)

diff --git a/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir b/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
index ba1a15bf773d2ef..9cb1049f91bcbdf 100644
--- a/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
+++ b/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
@@ -546,7 +546,7 @@ func.func private @_QPwork()
 // CHECK:          }
 // CHECK:          llvm.return
 // CHECK:        }
-// CHECK:        llvm.func @_QPwork() attributes {sym_visibility = "private"}
+// CHECK:        llvm.func private @_QPwork()
 // CHECK:      }
 
 // -----
@@ -591,7 +591,7 @@ func.func private @_QFPdo_work(!fir.ref<i32>)
 // CHECK:          }
 // CHECK:          llvm.return
 // CHECK:        }
-// CHECK:        llvm.func @_QFPdo_work(!llvm.ptr<i32>)
+// CHECK:        llvm.func private @_QFPdo_work(!llvm.ptr<i32>)
 // CHECK:      }
 
 // -----
diff --git a/flang/test/Fir/convert-to-llvm.fir b/flang/test/Fir/convert-to-llvm.fir
index 30695f115a60bd8..a1f032c204742cc 100644
--- a/flang/test/Fir/convert-to-llvm.fir
+++ b/flang/test/Fir/convert-to-llvm.fir
@@ -2264,7 +2264,7 @@ func.func @test_rebox_1(%arg0: !fir.box<!fir.array<?x?xf32>>) {
   fir.call @bar1(%3) : (!fir.box<!fir.array<?xf32>>) -> ()
   return
 }
-//CHECK-LABEL:  llvm.func @bar1
+//CHECK-LABEL:  llvm.func private @bar1
 //CHECK-LABEL:  llvm.func @test_rebox_1
 //CHECK-SAME:   %[[ARG0:.*]]: !llvm.ptr<struct<(ptr<f32>, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>>
 //CHECK:    %[[ONE_1:.*]] = llvm.mlir.constant(1 : i32) : i32
@@ -2337,7 +2337,7 @@ func.func @foo(%arg0: !fir.box<!fir.array<?x!fir.type<t{i:i32,c:!fir.char<1,10>}
   return
 }
 
-//CHECK: llvm.func @bar(!llvm.ptr<struct<(ptr<i8>, i64, i32, i8, i8, i8, i8, array<1 x array<3 x i64>>)>>) attributes {sym_visibility = "private"}
+//CHECK: llvm.func private @bar(!llvm.ptr<struct<(ptr<i8>, i64, i32, i8, i8, i8, i8, array<1 x array<3 x i64>>)>>)
 //CHECK-LABEL: llvm.func @foo
 //CHECK-SAME: %[[ARG0:.*]]: !llvm.ptr<struct<(ptr<struct<"t", (i32, array<10 x i8>)>>, i64, i32, i8, i8, i8, i8, array<1 x array<3 x i64>>, ptr<i8>, array<1 x i64>)>>
 //CHECK:   %[[ONE:.*]] = llvm.mlir.constant(1 : i32) : i32
diff --git a/flang/test/Fir/external-mangling.fir b/flang/test/Fir/external-mangling.fir
index 573bc9fecc55520..b04e3aac461d00b 100644
--- a/flang/test/Fir/external-mangling.fir
+++ b/flang/test/Fir/external-mangling.fir
@@ -49,8 +49,8 @@ func.func private @_QPbar2(!fir.ref<f32>)
 
 // LLVMIR-UNDER: llvm.mlir.global common @a_(dense<0> : vector<4xi8>) {{.*}} : !llvm.array<4 x i8>
 // LLVMIR-UNDER: llvm.mlir.global common @__BLNK__(dense<0> : vector<4xi8>) {{.*}}  : !llvm.array<4 x i8>
-// LLVMIR-UNDER: llvm.func @bar_(!llvm.ptr<i32>) attributes {sym_visibility = "private"}
-// LLVMIR-UNDER: llvm.func @bar2_(!llvm.ptr<f32>) attributes {sym_visibility = "private"}
+// LLVMIR-UNDER: llvm.func private @bar_(!llvm.ptr<i32>)
+// LLVMIR-UNDER: llvm.func private @bar2_(!llvm.ptr<f32>)
 
 // LLVMIR-NOUNDER: %{{.*}} = llvm.mlir.addressof @a : !llvm.ptr<array<4 x i8>>
 // LLVMIR-NOUNDER: %{{.*}} = llvm.mlir.addressof @__BLNK__ : !llvm.ptr<array<4 x i8>>
@@ -59,8 +59,8 @@ func.func private @_QPbar2(!fir.ref<f32>)
 
 // LLVMIR-NOUNDER: llvm.mlir.global common @a(dense<0> : vector<4xi8>) {{.*}} : !llvm.array<4 x i8>
 // LLVMIR-NOUNDER: llvm.mlir.global common @__BLNK__(dense<0> : vector<4xi8>) {{.*}}  : !llvm.array<4 x i8>
-// LLVMIR-NOUNDER: llvm.func @bar(!llvm.ptr<i32>) attributes {sym_visibility = "private"}
-// LLVMIR-NOUNDER: llvm.func @bar2(!llvm.ptr<f32>) attributes {sym_visibility = "private"}
+// LLVMIR-NOUNDER: llvm.func private @bar(!llvm.ptr<i32>)
+// LLVMIR-NOUNDER: llvm.func private @bar2(!llvm.ptr<f32>)
 
 // ----- 
 
diff --git a/flang/test/Fir/rename-msvc-libm.fir b/flang/test/Fir/rename-msvc-libm.fir
index 1c1f34433bb8246..c81ed54b901031b 100644
--- a/flang/test/Fir/rename-msvc-libm.fir
+++ b/flang/test/Fir/rename-msvc-libm.fir
@@ -5,7 +5,7 @@
 
 func.func private @hypotf(f32, f32) -> f32
 
-// CHECK: llvm.func @[[HYPOTF]](f32, f32) -> f32
+// CHECK: llvm.func private @[[HYPOTF]](f32, f32) -> f32
 
 func.func @call_hypotf(%arg0 : f32, %arg1 : f32) -> f32 {
   %0 = fir.call @hypotf(%arg0, %arg1) : (f32, f32) -> f32
diff --git a/flang/test/Fir/tbaa.fir b/flang/test/Fir/tbaa.fir
index eabc9f30127fa3d..80435951ae42f6c 100644
--- a/flang/test/Fir/tbaa.fir
+++ b/flang/test/Fir/tbaa.fir
@@ -194,9 +194,9 @@ module {
 // CHECK:           %[[VAL_64:.*]] = llvm.call @_FortranAioEndIoStatement(%[[VAL_10]]) {fastmathFlags = #llvm.fastmath<contract>} : (!llvm.ptr<i8>) -> i32
 // CHECK:           llvm.return
 // CHECK:         }
-// CHECK:         llvm.func @_FortranAioBeginExternalListOutput(i32, !llvm.ptr<i8>, i32) -> !llvm.ptr<i8> attributes {fir.io, fir.runtime, sym_visibility = "private"}
-// CHECK:         llvm.func @_FortranAioOutputDescriptor(!llvm.ptr<i8>, !llvm.ptr<struct<(ptr<struct<()>>, i64, i32, i8, i8, i8, i8, ptr<i8>, array<1 x i64>)>>) -> i1 attributes {fir.io, fir.runtime, sym_visibility = "private"}
-// CHECK:         llvm.func @_FortranAioEndIoStatement(!llvm.ptr<i8>) -> i32 attributes {fir.io, fir.runtime, sym_visibility = "private"}
+// CHECK:         llvm.func private @_FortranAioBeginExternalListOutput(i32, !llvm.ptr<i8>, i32) -> !llvm.ptr<i8> attributes {fir.io, fir.runtime}
+// CHECK:         llvm.func private @_FortranAioOutputDescriptor(!llvm.ptr<i8>, !llvm.ptr<struct<(ptr<struct<()>>, i64, i32, i8, i8, i8, i8, ptr<i8>, array<1 x i64>)>>) -> i1 attributes {fir.io, fir.runtime}
+// CHECK:         llvm.func private @_FortranAioEndIoStatement(!llvm.ptr<i8>) -> i32 attributes {fir.io, fir.runtime}
 
 // CHECK-LABEL:   llvm.mlir.global linkonce constant @_QQcl.2E2F64756D6D792E66393000() comdat(@__llvm_comdat::@_QQcl.2E2F64756D6D792E66393000) {addr_space = 0 : i32} : !llvm.array<12 x i8> {
 // CHECK:           %[[VAL_0:.*]] = llvm.mlir.constant("./dummy.f90\00") : !llvm.array<12 x i8>
diff --git a/flang/test/Intrinsics/math-codegen.fir b/flang/test/Intrinsics/math-codegen.fir
index 62d841253075e80..466746122a22fb2 100644
--- a/flang/test/Intrinsics/math-codegen.fir
+++ b/flang/test/Intrinsics/math-codegen.fir
@@ -1474,8 +1474,8 @@ func.func private @pow(f64, f64) -> f64
 // CHECK-NOCOMDAT-NOT: llvm.comdat_selector @__mlir_math_ipowi_i32 any
 // CHECK: @_QPtest_int4
 // CHECK: llvm.call @__mlir_math_ipowi_i32({{%[A-Za-z0-9._]+}}, {{%[A-Za-z0-9._]+}}) : (i32, i32) -> i32
-// CHECK-COMDAT: llvm.func linkonce_odr @__mlir_math_ipowi_i32(%arg0: i32, %arg1: i32) -> i32 comdat(@__llvm_comdat::@__mlir_math_ipowi_i32)
-// CHECK-NOCOMDAT: llvm.func linkonce_odr @__mlir_math_ipowi_i32(%arg0: i32, %arg1: i32) -> i32
+// CHECK-COMDAT: llvm.func private linkonce_odr @__mlir_math_ipowi_i32(%arg0: i32, %arg1: i32) -> i32 comdat(@__llvm_comdat::@__mlir_math_ipowi_i32)
+// CHECK-NOCOMDAT: llvm.func private linkonce_odr @__mlir_math_ipowi_i32(%arg0: i32, %arg1: i32) -> i32
 
 func.func @_QPtest_int4(%arg0: !fir.ref<i32> {fir.bindc_name = "x"}, %arg1: !fir.ref<i32> {fir.bindc_name = "y"}, %arg2: !fir.ref<i32> {fir.bindc_name = "z"}) {
   %0 = fir.load %arg0 : !fir.ref<i32>
diff --git a/flang/test/Lower/intrinsic-procedure-wrappers.f90 b/flang/test/Lower/intrinsic-procedure-wrappers.f90
index 210cfb413430164..f35cf2f0ccd38f8 100644
--- a/flang/test/Lower/intrinsic-procedure-wrappers.f90
+++ b/flang/test/Lower/intrinsic-procedure-wrappers.f90
@@ -7,4 +7,4 @@ function foo(x)
   foo = acos(x)
 end function
 
-! CHECK: llvm.func internal @fir.acos.contract.f32.f32
+! CHECK: llvm.func private internal @fir.acos.contract.f32.f32
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
index 25209ce4497455e..8745d14c8d48318 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
@@ -1395,6 +1395,7 @@ def LLVM_LLVMFuncOp : LLVM_Op<"func", [
 
   let arguments = (ins
     StrAttr:$sym_name,
+    OptionalAttr<StrAttr>:$sym_visibility,
     TypeAttrOf<LLVM_FunctionType>:$function_type,
     DefaultValuedAttr<Linkage, "Linkage::External">:$linkage,
     UnitAttr:$dso_local,
diff --git a/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp b/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
index d52f01880282e1a..b88911c0458108a 100644
--- a/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
+++ b/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
@@ -73,44 +73,40 @@ static bool shouldUseBarePtrCallConv(Operation *op,
 }
 
 /// 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,
+/// `LLVMFuncOp::build`.
+static void filterFuncAttributes(func::FuncOp func,
                                  SmallVectorImpl<NamedAttribute> &result) {
-  for (const NamedAttribute &attr : func->getAttrs()) {
-    if (attr.getName() == SymbolTable::getSymbolAttrName() ||
-        attr.getName() == func.getFunctionTypeAttrName() ||
-        attr.getName() == linkageAttrName ||
+  for (const NamedAttribute &attr : func->getDiscardableAttrs()) {
+    if (attr.getName() == linkageAttrName ||
         attr.getName() == varargsAttrName ||
-        attr.getName() == LLVM::LLVMDialect::getReadnoneAttrName() ||
-        (filterArgAndResAttrs &&
-         (attr.getName() == func.getArgAttrsAttrName() ||
-          attr.getName() == func.getResAttrsAttrName())))
+        attr.getName() == LLVM::LLVMDialect::getReadnoneAttrName())
       continue;
     result.push_back(attr);
   }
 }
 
-/// Adds a an empty set of argument attributes for the newly added argument in
-/// front of the existing ones.
-static void prependEmptyArgAttr(OpBuilder &builder,
-                                SmallVectorImpl<NamedAttribute> &newFuncAttrs,
-                                func::FuncOp func) {
-  auto argAttrs = func.getArgAttrs();
-  // Nothing to do when there were no arg attrs beforehand.
-  if (!argAttrs)
-    return;
-
-  size_t numArguments = func.getNumArguments();
-  SmallVector<Attribute> newArgAttrs;
-  newArgAttrs.reserve(numArguments + 1);
-  // Insert empty dictionary for the new argument.
-  newArgAttrs.push_back(builder.getDictionaryAttr({}));
-
-  llvm::append_range(newArgAttrs, *argAttrs);
-  auto newNamedAttr = builder.getNamedAttr(func.getArgAttrsAttrName(),
-                                           builder.getArrayAttr(newArgAttrs));
-  newFuncAttrs.push_back(newNamedAttr);
+/// Propagate argument/results attributes.
+static void propagateArgResAttrs(OpBuilder &builder, bool resultStructType,
+                                 func::FuncOp funcOp,
+                                 LLVM::LLVMFuncOp wrapperFuncOp) {
+  auto argAttrs = funcOp.getArgAttrs();
+  if (!resultStructType) {
+    if (auto resAttrs = funcOp.getAllResultAttrs())
+      wrapperFuncOp.setAllResultAttrs(resAttrs);
+    if (argAttrs)
+      wrapperFuncOp.setAllArgAttrs(*argAttrs);
+  } else {
+    SmallVector<Attribute> argAttributes;
+    // Only modify the argument and result attributes when the result is now
+    // an argument.
+    if (argAttrs) {
+      argAttributes.push_back(builder.getDictionaryAttr({}));
+      argAttributes.append(argAttrs->begin(), argAttrs->end());
+      wrapperFuncOp.setAllArgAttrs(argAttributes);
+    }
+  }
+  if (funcOp.getSymVisibilityAttr())
+    wrapperFuncOp.setSymVisibility(funcOp.getSymVisibilityAttr());
 }
 
 /// Creates an auxiliary function with pointer-to-memref-descriptor-struct
@@ -129,18 +125,14 @@ static void wrapForExternalCallers(OpBuilder &rewriter, Location loc,
   auto [wrapperFuncType, resultStructType] =
       typeConverter.convertFunctionTypeCWrapper(type);
 
-  SmallVector<NamedAttribute, 4> attributes;
-  // Only modify the argument and result attributes when the result is now an
-  // argument.
-  if (resultStructType)
-    prependEmptyArgAttr(rewriter, attributes, funcOp);
-  filterFuncAttributes(
-      funcOp, /*filterArgAndResAttrs=*/static_cast<bool>(resultStructType),
-      attributes);
+  SmallVector<NamedAttribute> attributes;
+  filterFuncAttributes(funcOp, attributes);
+
   auto wrapperFuncOp = rewriter.create<LLVM::LLVMFuncOp>(
       loc, llvm::formatv("_mlir_ciface_{0}", funcOp.getName()).str(),
       wrapperFuncType, LLVM::Linkage::External, /*dsoLocal=*/false,
       /*cconv=*/LLVM::CConv::C, /*comdat=*/nullptr, attributes);
+  propagateArgResAttrs(rewriter, !!resultStructType, funcOp, wrapperFuncOp);
 
   OpBuilder::InsertionGuard guard(rewriter);
   rewriter.setInsertionPointToStart(wrapperFuncOp.addEntryBlock());
@@ -199,19 +191,14 @@ static void wrapExternalFunction(OpBuilder &builder, Location loc,
   assert(wrapperType && "unexpected type conversion failure");
 
   SmallVector<NamedAttribute, 4> attributes;
-  // Only modify the argument and result attributes when the result is now an
-  // argument.
-  if (resultStructType)
-    prependEmptyArgAttr(builder, attributes, funcOp);
-  filterFuncAttributes(
-      funcOp, /*filterArgAndResAttrs=*/static_cast<bool>(resultStructType),
-      attributes);
+  filterFuncAttributes(funcOp, attributes);
 
   // Create the auxiliary function.
   auto wrapperFunc = builder.create<LLVM::LLVMFuncOp>(
       loc, llvm::formatv("_mlir_ciface_{0}", funcOp.getName()).str(),
       wrapperType, LLVM::Linkage::External, /*dsoLocal=*/false,
       /*cconv=*/LLVM::CConv::C, /*comdat=*/nullptr, attributes);
+  propagateArgResAttrs(builder, !!resultStructType, funcOp, wrapperFunc);
 
   // The wrapper that we synthetize here should only be visible in this module.
   newFuncOp.setLinkage(LLVM::Linkage::Private);
@@ -351,21 +338,57 @@ struct FuncOpConversionBase : public ConvertOpToLLVMPattern<func::FuncOp> {
     if (!llvmType)
       return rewriter.notifyMatchFailure(funcOp, "signature conversion failed");
 
+    // Create an LLVM function, use external linkage by default until MLIR
+    // functions have linkage.
+    LLVM::Linkage linkage = LLVM::Linkage::External;
+    if (funcOp->hasAttr(linkageAttrName)) {
+      auto attr =
+          dyn_cast<mlir::LLVM::LinkageAttr>(funcOp->getAttr(linkageAttrName));
+      if (!attr) {
+        funcOp->emitError() << "Contains " << linkageAttrName
+                            << " attribute not of type LLVM::LinkageAttr";
+        return rewriter.notifyMatchFailure(
+            funcOp, "Contains linkage attribute not of type LLVM::LinkageAttr");
+      }
+      linkage = attr.getLinkage();
+    }
+
+    SmallVector<NamedAttribute, 4> attributes;
+    filterFuncAttributes(funcOp, attributes);
+    auto newFuncOp = rewriter.create<LLVM::LLVMFuncOp>(
+        funcOp.getLoc(), funcOp.getName(), llvmType, linkage,
+        /*dsoLocal=*/false, /*cconv=*/LLVM::CConv::C, /*comdat=*/nullptr,
+        attributes);
+
+    if (funcOp.getSymVisibilityAttr())
+      newFuncOp.setSymVisibility(funcOp.getSymVisibilityAttr());
+
+    // Create a memory effect attribute corresponding to readnone.
+    StringRef readnoneAttrName = LLVM::LLVMDialect::getReadnoneAttrName();
+    if (funcOp->hasAttr(readnoneAttrName)) {
+      auto attr = funcOp->getAttrOfType<UnitAttr>(readnoneAttrName);
+      if (!attr) {
+        funcOp->emitError() << "Contains " << readnoneAttrName
+                            << " attribute not of type UnitAttr";
+        return rewriter.notifyMatchFailure(
+            funcOp, "Contains readnone attribute not of type UnitAttr");
+      }
+      auto memoryAttr = LLVM::MemoryEffectsAttr::get(
+          rewriter.getContext(),
+          {LLVM::ModRefInfo::NoModRef, LLVM::ModRefInfo::NoModRef,
+           LLVM::ModRefInfo::NoModRef});
+      newFuncOp.setMemoryAttr(memoryAttr);
+    }
+
     // Propagate argument/result attributes to all converted arguments/result
     // obtained after converting a given original argument/result.
-    SmallVector<NamedAttribute, 4> attributes;
-    filterFuncAttributes(funcOp, /*filterArgAndResAttrs=*/true, attributes);
     if (ArrayAttr resAttrDicts = funcOp.getAllResultAttrs()) {
       assert(!resAttrDicts.empty() && "expected array to be non-empty");
-      auto newResAttrDicts =
-          (funcOp.getNumResults() == 1)
-              ? resAttrDicts
-              : rewriter.getArrayAttr(rewriter.getDictionaryAttr({}));
-      attributes.push_back(
-          rewriter.getNamedAttr(funcOp.getResAttrsAttrName(), newResAttrDicts));
+      if (funcOp.getNumResults() == 1)
+        newFuncOp.setAllResultAttrs(resAttrDicts);
     }
     if (ArrayAttr argAttrDicts = funcOp.getAllArgAttrs()) {
-      SmallVector<Attribute, 4> newArgAttrs(
+      SmallVector<Attribute> newArgAttrs(
           cast<LLVM::LLVMFunctionType>(llvmType).getNumParams());
       for (unsigned i = 0, e = funcOp.getNumArguments(); i < e; ++i) {
         // Some LLVM IR attribute have a type attached to them. During FuncOp ->
@@ -415,48 +438,10 @@ struct FuncOpConversionBase : public ConvertOpToLLVMPattern<func::FuncOp> {
           newArgAttrs[mapping->inputNo + j] =
               DictionaryAttr::get(rewriter.getContext(), {});
       }
-      attributes.push_back(rewriter.getNamedAttr(
-          funcOp.getArgAttrsAttrName(), rewriter.getArrayAttr(newArgAttrs)));
-    }
-
-    // Create an LLVM function, use external linkage by default until MLIR
-    // functions have linkage.
-    LLVM::Linkage linkage = LLVM::Linkage::External;
-    if (funcOp->hasAttr(linkageAttrName)) {
-      auto attr =
-          dyn_cast<mlir::LLVM::LinkageAttr>(funcOp->getAttr(linkageAttrName));
-      if (!attr) {
-        funcOp->emitError() << "Contains " << linkageAttrName
-                            << " attribute not of type LLVM::LinkageAttr";
-        return rewriter.notifyMatchFailure(
-            funcOp, "Contains linkage attribute not of type LLVM::LinkageAttr");
-      }
-      linkage = attr.getLinkage();
+      if (!newArgAttrs.empty())
+        newFuncOp.setAllArgAttrs(rewriter.getArrayAttr(newArgAttrs));
     }
 
-    // Create a memory effect attribute corresponding to readnone.
-    StringRef readnoneAttrName = LLVM::LLVMDialect::getReadnoneAttrName();
-    LLVM::MemoryEffectsAttr memoryAttr = {};
-    if (funcOp->hasAttr(readnoneAttrName)) {
-      auto attr = funcOp->getAttrOfType<UnitAttr>(readnoneAttrName);
-      if (!attr) {
-        funcOp->emitError() << "Contains " << readnoneAttrName
-                            << " attribute not of type UnitAttr";
-        return rewriter.notifyMatchFailure(
-            funcOp, "Contains readnone attribute not of type UnitAttr");
-      }
-      memoryAttr = LLVM::MemoryEffectsAttr::get(rewriter.getContext(),
-                                                {LLVM::ModRefInfo::NoModRef,
-                                                 LLVM::ModRefInfo::NoModRef,
-                                                 LLVM::ModRefInfo::NoModRef});
-    }
-    auto newFuncOp = rewriter.create<LLVM::LLVMFuncOp>(
-        funcOp.getLoc(), funcOp.getName(), llvmType, linkage,
-        /*dsoLocal=*/false, /*cconv=*/LLVM::CConv::C, /*comdat=*/nullptr,
-        attributes);
-    // If the memory attribute was created, add it to the function.
-    if (memoryAttr)
-      newFuncOp.setMemoryAttr(memoryAttr);
     rewriter.inlineRegionBefore(funcOp.getBody(), newFuncOp.getBody(),
                                 newFuncOp.end());
     if (failed(rewriter.convertRegionTypes(&newFuncOp.getBody(), *typeConverter,
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index 95c04098d05fc2f..a3413cc696ece9a 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -2081,6 +2081,7 @@ static LogicalResult verifyComdat(Operation *op,
 // inferred from the value of the string as [strlen(value) x i8].
 ParseResult GlobalOp::parse(OpAsmParser &parser, OperationState &result) {
   MLIRContext *ctx = parser.getContext();
+
   // Parse optional linkage, default to External.
   result.addAttribute(getLinkageAttrName(result.name),
                       LLVM::LinkageAttr::get(
@@ -2441,6 +2442,9 @@ buildLLVMFunctionType(OpAsmParser &parser, SMLoc loc, ArrayRef<Type> inputs,
 //                function-body
 //
 ParseResult LLVMFuncOp::parse(OpAsmParser &parser, OperationState &result) {
+  // Parse visibility.
+  (void)impl::parseOptionalVisibilityKeyword(parser, result.attributes);
+
   // Default to external linkage if no keyword is provided.
   result.addAttribute(
       getLinkageAttrName(result.name),
@@ -2531,11 +2535,17 @@ ParseResult LLVMFuncOp::parse(OpAsmParser &parser, OperationState &result) {
 // the external linkage since it is the default value.
 void LLVMFuncOp::print(OpAsmPrinter &p) {
   p << ' ';
+
   if (getLinkage() != LLVM::Linkage::External)
     p << stringifyLinkage(getLinkage()) << ' ';
+  // Print visibility, unless it's the default.
   StringRef visibility = stringifyVisibility(getVisibility_());
-  if (!visibility.empty())
-    p << visibility << ' ';
+  if (!visibility.empty()) {
+    if (isDeclaration() && visibility != "private")
+      p << visibility << ' ';
+    if (!isDeclaration() && visibility != "public")
+      p << visibility << ' ';
+  }
   if (auto unnamedAddr = getUnnamedAddr()) {
     StringRef str = stringifyUnnamedAddr(*unnamedAddr);
     if (!str.empty())
@@ -2573,8 +2583,8 @@ void LLVMFuncOp::print(OpAsmPrinter &p) {
       p, *this,
       {getFunctionTypeAttrName(), getArgAttrsAttrName(), getResAttrsAttrName(),
        getLinkageAttrName(), getCConvAttrName(), getVisibility_AttrName(),
-       getComdatAttrName(), getUnnamedAddrAttrName(),
-       getVscaleRangeAttrName()});
+       getComdatAttrName(), getUnnamedAddrAttrName(), getVscaleRangeAttrName(),
+       getSymVisibilityAttrName()});
 
   // Print the body if this is not an external function.
   Region &body = getBody();



More information about the Mlir-commits mailing list