Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MLIR conversion to LLVMFunc to account better for properties #67406

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

joker-eph
Copy link
Collaborator

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

@joker-eph joker-eph requested a review from ftynse September 26, 2023 08:52
@llvmbot llvmbot added the mlir label Sep 26, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 26, 2023

@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
Copy link
Contributor

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

@joker-eph joker-eph force-pushed the LLVMFuncConversionProperties branch from 4d7298b to 091639d Compare September 26, 2023 09:33
@joker-eph
Copy link
Collaborator Author

Thanks @allatit23 ; I addressed it!

@joker-eph joker-eph requested review from jpienaar and gysit September 26, 2023 20:00
Copy link
Contributor

@gysit gysit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing properties.

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

mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp Outdated Show resolved Hide resolved
mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Sep 27, 2023

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

@joker-eph joker-eph force-pushed the LLVMFuncConversionProperties branch 2 times, most recently from 4497dcf to b325b83 Compare September 27, 2023 08:40
@joker-eph joker-eph force-pushed the LLVMFuncConversionProperties branch from b325b83 to 91312a8 Compare September 27, 2023 09:29
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp labels Sep 27, 2023
@joker-eph
Copy link
Collaborator Author

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

@allatit23
Copy link
Contributor

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

@ftynse
Copy link
Member

ftynse commented Oct 5, 2023

@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.

@ftynse ftynse requested a review from gysit October 5, 2023 11:38
@gysit
Copy link
Contributor

gysit commented Oct 5, 2023

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
Copy link
Collaborator Author

joker-eph commented Oct 9, 2023

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

@joker-eph joker-eph force-pushed the LLVMFuncConversionProperties branch 4 times, most recently from 9717108 to ac10033 Compare October 9, 2023 06:49
Copy link
Contributor

@gysit gysit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

mlir/test/Conversion/FuncToLLVM/convert-funcs.mlir Outdated Show resolved Hide resolved
mlir/test/Dialect/LLVMIR/func.mlir Outdated Show resolved Hide resolved
@joker-eph joker-eph force-pushed the LLVMFuncConversionProperties branch from ac10033 to 325f69f Compare October 9, 2023 07:39
Change the attribute propagation to handle only discardable attributes,
inherent attribute are handled directly and args/results through the
interface.
@joker-eph joker-eph force-pushed the LLVMFuncConversionProperties branch from 325f69f to c4c5d7d Compare October 9, 2023 07:40
@joker-eph joker-eph merged commit cb550c1 into llvm:main Oct 9, 2023
@joker-eph joker-eph deleted the LLVMFuncConversionProperties branch October 9, 2023 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang:openmp flang Flang issues not falling into any other category mlir:llvm mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants