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

[mlir] Speed up FuncToLLVM using a SymbolTable #68082

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

tdanyluk
Copy link
Contributor

@tdanyluk tdanyluk commented Oct 3, 2023

We have a project where this saves 23% of the compilation time.

This means using hashmaps instead of searching in linked lists.

@llvmbot llvmbot added the mlir label Oct 3, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 3, 2023

@llvm/pr-subscribers-mlir

Changes

We 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:

  • (modified) mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp (+6-2)
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

@ftynse
Copy link
Member

ftynse commented Oct 3, 2023

Does this need a symbol table collection? Normally, there are no nested symbol tables at this level.

Also please document how/why this works.

@ftynse ftynse self-requested a review October 3, 2023 09:24
Copy link
Contributor

@chsigg chsigg left a 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.

@chsigg chsigg self-requested a review October 3, 2023 09:27
@chsigg
Copy link
Contributor

chsigg commented Oct 3, 2023

I've looked again and it seems to me that this could go wrong if the caller of populateFuncToLLVMConversionPatterns() (presumably some module pass) inserted additional functions+calls between applying this pattern. The symbol table wouldn't know about this and callee would be nullptr even if the function now existed.

@tdanyluk tdanyluk force-pushed the speed_up_func_to_llvm branch from 0181f18 to 63c1700 Compare October 3, 2023 15:24
@tdanyluk
Copy link
Contributor Author

tdanyluk commented Oct 3, 2023

Thanks for the quick comments you both.
The solution was indeed not perfect: it broke some tests.
Now my updated solution doesn't.

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 SymName (both func::FuncOp and LLVM::LLVMFuncOp).
So I opted to simply precalculate a map of function names to booleans.

This only speeds up ConvertFuncToLLVMPass, it leaves the existing logic for external callers of populateFuncToLLVMConversionPatterns.

BTW, sorry, this is my first MLIR commit, please LMK what should I improve.

@tdanyluk tdanyluk changed the title [mlir] Speed up FuncToLLVM: CallOpLowering using SymbolTableCollection [mlir] Speed up FuncToLLVM: CallOpLowering using a hashtable Oct 3, 2023
mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp Outdated Show resolved Hide resolved
mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp Outdated Show resolved Hide resolved
mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp Outdated Show resolved Hide resolved
mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp Outdated Show resolved Hide resolved
@tdanyluk tdanyluk force-pushed the speed_up_func_to_llvm branch from 63c1700 to 078df36 Compare October 3, 2023 17:49
@tdanyluk tdanyluk force-pushed the speed_up_func_to_llvm branch 2 times, most recently from 42e7584 to 3a29249 Compare October 4, 2023 09:02
@tdanyluk
Copy link
Contributor Author

tdanyluk commented Oct 4, 2023

I rewrote the changeset using the pattern recommended by @joker-eph. I think that it's ready for review.

@tdanyluk tdanyluk changed the title [mlir] Speed up FuncToLLVM: CallOpLowering using a hashtable [mlir] Speed up FuncToLLVM using a SymbolTable Oct 4, 2023
@tdanyluk tdanyluk requested review from joker-eph and ftynse October 4, 2023 09:20
Copy link
Member

@ftynse ftynse left a comment

Choose a reason for hiding this comment

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

Nice!

mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp Outdated Show resolved Hide resolved
@tdanyluk tdanyluk force-pushed the speed_up_func_to_llvm branch from 3a29249 to d81c4be Compare October 4, 2023 10:46
@tdanyluk tdanyluk force-pushed the speed_up_func_to_llvm branch 3 times, most recently from d40486d to b025d1f Compare October 5, 2023 08:26

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
This means that we do hashmap lookups instead of linear lookups.
We have a project where this saves 23% of the compilation time.
@tdanyluk tdanyluk force-pushed the speed_up_func_to_llvm branch from b025d1f to 684b0f7 Compare October 5, 2023 08:27
@tdanyluk
Copy link
Contributor Author

tdanyluk commented Oct 5, 2023

Exposed the symbolTable parameter as requested by @joker-eph.

Copy link
Collaborator

@joker-eph joker-eph left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants