-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[mlir][LLVMIR] Add IFuncOp to LLVM dialect #147697
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
Changes from 1 commit
36b982f
2593954
1126fe2
21ccdf8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -71,6 +71,9 @@ class ModuleImport { | |||||
| /// Converts all aliases of the LLVM module to MLIR variables. | ||||||
| LogicalResult convertAliases(); | ||||||
|
|
||||||
| /// Converts all ifuncs of the LLVM module to MLIR variables. | ||||||
| LogicalResult convertIFuncs(); | ||||||
|
|
||||||
| /// Converts the data layout of the LLVM module to an MLIR data layout | ||||||
| /// specification. | ||||||
| LogicalResult convertDataLayout(); | ||||||
|
|
@@ -320,6 +323,8 @@ class ModuleImport { | |||||
| /// Converts an LLVM global alias variable into an MLIR LLVM dialect alias | ||||||
| /// operation if a conversion exists. Otherwise, returns failure. | ||||||
| LogicalResult convertAlias(llvm::GlobalAlias *alias); | ||||||
| // Converts an LLVM global ifunc into an MLIR LLVM diaeclt ifunc operation | ||||||
|
||||||
| // Converts an LLVM global ifunc into an MLIR LLVM diaeclt ifunc operation | |
| // Converts an LLVM global ifunc into an MLIR LLVM dialect ifunc operation. |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -139,6 +139,17 @@ static RetTy parseOptionalLLVMKeyword(OpAsmParser &parser, | |||||||||||
| return static_cast<RetTy>(index); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| static void printLLVMLinkage(OpAsmPrinter &p, Operation *, LinkageAttr val) { | ||||||||||||
| p << stringifyLinkage(val.getLinkage()); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| static OptionalParseResult parseLLVMLinkage(OpAsmParser &p, LinkageAttr &val) { | ||||||||||||
| val = LinkageAttr::get( | ||||||||||||
| p.getContext(), | ||||||||||||
| parseOptionalLLVMKeyword<LLVM::Linkage>(p, LLVM::Linkage::External)); | ||||||||||||
| return success(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| //===----------------------------------------------------------------------===// | ||||||||||||
| // Operand bundle helpers. | ||||||||||||
| //===----------------------------------------------------------------------===// | ||||||||||||
|
|
@@ -1175,14 +1186,17 @@ LogicalResult CallOp::verifySymbolUses(SymbolTableCollection &symbolTable) { | |||||||||||
| return emitOpError() | ||||||||||||
| << "'" << calleeName.getValue() | ||||||||||||
| << "' does not reference a symbol in the current scope"; | ||||||||||||
| auto fn = dyn_cast<LLVMFuncOp>(callee); | ||||||||||||
| if (!fn) | ||||||||||||
| return emitOpError() << "'" << calleeName.getValue() | ||||||||||||
| << "' does not reference a valid LLVM function"; | ||||||||||||
|
|
||||||||||||
| if (failed(verifyCallOpDebugInfo(*this, fn))) | ||||||||||||
| return failure(); | ||||||||||||
| fnType = fn.getFunctionType(); | ||||||||||||
| if (auto fn = dyn_cast<LLVMFuncOp>(callee)) { | ||||||||||||
| if (failed(verifyCallOpDebugInfo(*this, fn))) | ||||||||||||
| return failure(); | ||||||||||||
| fnType = fn.getFunctionType(); | ||||||||||||
| } else if (auto ifunc = dyn_cast<IFuncOp>(callee)) { | ||||||||||||
| fnType = ifunc.getIFuncType(); | ||||||||||||
| } else { | ||||||||||||
| return emitOpError() | ||||||||||||
| << "'" << calleeName.getValue() | ||||||||||||
| << "' does not reference a valid LLVM function or IFunc"; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| LLVMFunctionType funcType = llvm::dyn_cast<LLVMFunctionType>(fnType); | ||||||||||||
|
|
@@ -2038,14 +2052,6 @@ LogicalResult ReturnOp::verify() { | |||||||||||
| // LLVM::AddressOfOp. | ||||||||||||
| //===----------------------------------------------------------------------===// | ||||||||||||
|
|
||||||||||||
| static Operation *parentLLVMModule(Operation *op) { | ||||||||||||
| Operation *module = op->getParentOp(); | ||||||||||||
| while (module && !satisfiesLLVMModule(module)) | ||||||||||||
| module = module->getParentOp(); | ||||||||||||
| assert(module && "unexpected operation outside of a module"); | ||||||||||||
| return module; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| GlobalOp AddressOfOp::getGlobal(SymbolTableCollection &symbolTable) { | ||||||||||||
| return dyn_cast_or_null<GlobalOp>( | ||||||||||||
| symbolTable.lookupSymbolIn(parentLLVMModule(*this), getGlobalNameAttr())); | ||||||||||||
|
|
@@ -2061,6 +2067,11 @@ AliasOp AddressOfOp::getAlias(SymbolTableCollection &symbolTable) { | |||||||||||
| symbolTable.lookupSymbolIn(parentLLVMModule(*this), getGlobalNameAttr())); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| IFuncOp AddressOfOp::getIFunc(SymbolTableCollection &symbolTable) { | ||||||||||||
| return dyn_cast_or_null<IFuncOp>( | ||||||||||||
| symbolTable.lookupSymbolIn(parentLLVMModule(*this), getGlobalNameAttr())); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| LogicalResult | ||||||||||||
| AddressOfOp::verifySymbolUses(SymbolTableCollection &symbolTable) { | ||||||||||||
| Operation *symbol = | ||||||||||||
|
|
@@ -2069,10 +2080,11 @@ AddressOfOp::verifySymbolUses(SymbolTableCollection &symbolTable) { | |||||||||||
| auto global = dyn_cast_or_null<GlobalOp>(symbol); | ||||||||||||
| auto function = dyn_cast_or_null<LLVMFuncOp>(symbol); | ||||||||||||
| auto alias = dyn_cast_or_null<AliasOp>(symbol); | ||||||||||||
| auto ifunc = dyn_cast_or_null<IFuncOp>(symbol); | ||||||||||||
|
|
||||||||||||
| if (!global && !function && !alias) | ||||||||||||
| if (!global && !function && !alias && !ifunc) | ||||||||||||
| return emitOpError("must reference a global defined by 'llvm.mlir.global', " | ||||||||||||
| "'llvm.mlir.alias' or 'llvm.func'"); | ||||||||||||
| "'llvm.mlir.alias' or 'llvm.func' or 'llvm.mlir.ifunc'"); | ||||||||||||
|
|
||||||||||||
| LLVMPointerType type = getType(); | ||||||||||||
| if ((global && global.getAddrSpace() != type.getAddressSpace()) || | ||||||||||||
|
|
@@ -2682,6 +2694,56 @@ unsigned AliasOp::getAddrSpace() { | |||||||||||
| return ptrTy.getAddressSpace(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| //===----------------------------------------------------------------------===// | ||||||||||||
| // IFuncOp | ||||||||||||
| //===----------------------------------------------------------------------===// | ||||||||||||
|
|
||||||||||||
| void IFuncOp::build(OpBuilder &builder, OperationState &result, StringRef name, | ||||||||||||
| Type iFuncType, StringRef resolverName, Type resolverType, | ||||||||||||
| Linkage linkage, LLVM::Visibility visibility) { | ||||||||||||
| return build(builder, result, name, iFuncType, resolverName, resolverType, | ||||||||||||
| /* dso_local */ false, /* addr_space */ 0, linkage, | ||||||||||||
|
||||||||||||
| /* dso_local */ false, /* addr_space */ 0, linkage, | |
| /*dso_local=*/false, /*addr_space=*/0, linkage, |
nit: style
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.
| } | |
| LogicalResult IFuncOp::verifySymbolUses(SymbolTableCollection &symbolTable) { | |
| } | |
| LogicalResult IFuncOp::verifySymbolUses(SymbolTableCollection &symbolTable) { |
nit missing newline
Outdated
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.
That makes me believe the PR needs some more verifier tests!
Outdated
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.
| // Copying logic from llvm/lib/IR/Verifier.cpp | |
| // This matches LLVM IR verification logic, see from llvm/lib/IR/Verifier.cpp |
ultra nit: I suggest to avoid the term copying since it reminds of copy pasta :)
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -422,9 +422,18 @@ convertOperationImpl(Operation &opInst, llvm::IRBuilderBase &builder, | |||||
| ArrayRef<llvm::Value *> operandsRef(operands); | ||||||
| llvm::CallInst *call; | ||||||
| if (auto attr = callOp.getCalleeAttr()) { | ||||||
| call = | ||||||
| builder.CreateCall(moduleTranslation.lookupFunction(attr.getValue()), | ||||||
| operandsRef, opBundles); | ||||||
| if (llvm::Function *function = | ||||||
| moduleTranslation.lookupFunction(attr.getValue())) { | ||||||
| call = builder.CreateCall(function, operandsRef, opBundles); | ||||||
| } else { | ||||||
| Operation *module = parentLLVMModule(&opInst); | ||||||
|
||||||
| Operation *module = parentLLVMModule(&opInst); | |
| Operation *moduleOp = parentLLVMModule(&opInst); |
nit: Let's use module op to avoid the red color.
I wonder if it would make sense if LLVM::CallOp would have a getFunction / getIFunc helper similar to the addressOf operation. But I suspect this would likely be the only use here?
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.
I don't think even the AddressOfOp versions are used anywhere else, so based on that precedent it would make sense to add it to the CallOp.
I also just noticed that dsoLocalEquivalentOp also has similar helpers, so adding it for consistency makes sense.
Not 100 % sure if I should introduce the change in this PR though?
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.
I do not have super strong opinion here.
I commented bcs parentLLVMModule moved to a header. I guess the helpers could be an alternative to exposing this helper function. But yeah I leave it up to you.
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.
For functions and globals we put the linkage always before dso_local. Should we do this here as well?
Also was there a specific reason to use a DefaultValuedAttr for the linkage. AliasOp and GlobalOp always print the value which seems reasonable for such a relevant attribute (Or did you want to follow FuncOp?).
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.
Hm, I think I was mainly following what FuncOp and also what LLVM IR does.
I must have missed that Alias/Global Ops print it. I was looking at their parser and that allows for the missing linkage. I will change it so that the behaviour is the same as for Aliases/Globals.
I will also reorder the arguments to match the other operations.
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.
It is not a strict requirement, but I think it makes sense for this op to be similar to Aliases/Globals.