-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Conversation
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-mlir ChangesChange 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:
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 =
|
Seems that |
4d7298b
to
091639d
Compare
Thanks @allatit23 ; I addressed it! |
There was a problem hiding this 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?
✅ With the latest revision this PR passed the C/C++ code formatter. |
4497dcf
to
b325b83
Compare
b325b83
to
91312a8
Compare
@ftynse I added the |
hi, @joker-eph , seems that there are some failed UTs. Is there any update about this PR? Thanks. |
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. |
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? |
Looks like this will need more thought, so I'll stick with adding sym_visibility to LLVM:FuncOp for now. |
9717108
to
ac10033
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
ac10033
to
325f69f
Compare
Change the attribute propagation to handle only discardable attributes, inherent attribute are handled directly and args/results through the interface.
325f69f
to
c4c5d7d
Compare
Change the attribute propagation to handle only discardable attributes, inherent attribute are handled directly and args/results through the interface.