Update MLIR conversion to LLVMFunc to account better for properties

[joker-eph]

Change the attribute propagation to handle only discardable attributes, inherent attribute are handled directly and args/results through the interface.

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-flang-openmp
@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-mlir

Changes

Change the attribute propagation to handle only discardable attributes, inherent attribute are handled directly and args/results through the interface.

---

Full diff: https://github.com/llvm/llvm-project/pull/67406.diff

1 Files Affected:

• (modified) mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp (+31-45)

diff --git a/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp b/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
index 7de7f3cb9e36b06..3d71d6c4d78b57f 100644
--- a/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
+++ b/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
@@ -73,42 +73,37 @@ 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,
+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);
+    }
+  }
 }
 
 /// Creates an auxiliary function with pointer-to-memref-descriptor-struct
@@ -127,18 +122,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());
@@ -197,19 +188,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);
@@ -352,7 +338,7 @@ struct FuncOpConversionBase : public ConvertOpToLLVMPattern<func::FuncOp> {
     // 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);
+    filterFuncAttributes(funcOp, attributes);
     if (ArrayAttr resAttrDicts = funcOp.getAllResultAttrs()) {
       assert(!resAttrDicts.empty() && "expected array to be non-empty");
       auto newResAttrDicts =

[allatit23]

Seems that funcOp.getResAttrsAttrName() in line 349 and funcOp.getArgAttrsAttrName() in line 403 are missed?

[joker-eph]

Thanks @allatit23 ; I addressed it!

[gysit]

Thanks for pushing properties.

Looks good to me but I believe the symbol visibility needs to be lowered as well?

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[joker-eph]

@ftynse I added the sym_visibility to LLVMFuncOp, can you double check that it makes sense here?

[allatit23]

hi, @joker-eph , seems that there are some failed UTs. Is there any update about this PR? Thanks.

[ftynse]

@ftynse I added the sym_visibility to LLVMFuncOp, can you double check that it makes sense here?

I think we discussed making visibility a part of symbol interface and having it depend on the linkage attribute for LLVMFuncOp. That would be my preferred solution. However, @jpienaar had an argument about having private linkage yet public visibility (or vice versa) functions somehow, but I don't remember what it was. @gysit may also know better given that they have cases of converting from LLVM IR to the LLVM dialect and inlining at that level.

[gysit]

I think we discussed making visibility a part of symbol interface and having it depend on the linkage attribute for LLVMFuncOp. That would be my preferred solution. However, @jpienaar had an argument about having private linkage yet public visibility (or vice versa) functions somehow, but I don't remember what it was.

It would be nice if we find a consistent way of deriving the visibility from the linkage and maybe the fact if a function is a declaration. I remember there have been discussions about it before and it think it was not completely trivial to find a consistent solution. I think the SymbolOpVerifier checks that a declaration does not have public visibility. LLVM on the other hand verifies that a declaration has external linkage (or extern_weak). That means public != extern as one could assume. On the other hand, setting the visibility of all extern symbols to private is confusing since private linkage is exactly the opposite of extern linkage 🤯 (I hope I got that right...).

Another argument may be that we want to keep supporting scenarios where symbols need to have a configurable visibility. For example, when compiling for GPUs one may want to be able to have nested modules and symbols with a specific visibility? Coupling visibility and LLVM linkage may be too limiting in this case?

[joker-eph]

Looks like this will need more thought, so I'll stick with adding sym_visibility to LLVM:FuncOp for now.

[gysit]

LGTM!