-
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
[mlir] Speed up FuncToLLVM using a SymbolTable #68082
Conversation
@llvm/pr-subscribers-mlir ChangesWe have a project where this saves 23% of the compilation time. This means using hashmaps instead of searching in linked lists. Full diff: https://github.com/llvm/llvm-project/pull/68082.diff 1 Files Affected:
diff --git a/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp b/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
index 7de7f3cb9e36b06..b853181c618365d 100644
--- a/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
+++ b/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
@@ -601,19 +601,23 @@ struct CallOpInterfaceLowering : public ConvertOpToLLVMPattern<CallOpType> {
}
};
-struct CallOpLowering : public CallOpInterfaceLowering<func::CallOp> {
+class CallOpLowering : public CallOpInterfaceLowering<func::CallOp> {
+public:
using Super::Super;
LogicalResult
matchAndRewrite(func::CallOp callOp, OpAdaptor adaptor,
ConversionPatternRewriter &rewriter) const override {
bool useBarePtrCallConv = false;
- if (Operation *callee = SymbolTable::lookupNearestSymbolFrom(
+ if (Operation *callee = symbolTableCollection.lookupNearestSymbolFrom(
callOp, callOp.getCalleeAttr())) {
useBarePtrCallConv = shouldUseBarePtrCallConv(callee, getTypeConverter());
}
return matchAndRewriteImpl(callOp, adaptor, rewriter, useBarePtrCallConv);
}
+
+private:
+ mutable SymbolTableCollection symbolTableCollection;
};
struct CallIndirectOpLowering
|
Does this need a symbol table collection? Normally, there are no nested symbol tables at this level. Also please document how/why this works. |
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.
Thank you, this looks like a nice optimization.
But please look into Alex' comments.
I've looked again and it seems to me that this could go wrong if the caller of |
0181f18
to
63c1700
Compare
Thanks for the quick comments you both. It turned out that we can't really use symbol tables at this point, because during the conversion there can be multiple operations with the same This only speeds up BTW, sorry, this is my first MLIR commit, please LMK what should I improve. |
63c1700
to
078df36
Compare
42e7584
to
3a29249
Compare
I rewrote the changeset using the pattern recommended by @joker-eph. I think that it's ready for review. |
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.
Nice!
3a29249
to
d81c4be
Compare
d40486d
to
b025d1f
Compare
This means that we do hashmap lookups instead of linear lookups. We have a project where this saves 23% of the compilation time.
b025d1f
to
684b0f7
Compare
Exposed the |
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!
We have a project where this saves 23% of the compilation time.
This means using hashmaps instead of searching in linked lists.