Port refprune pass to NewPassManager by yashssh · Pull Request #1057 · numba/llvmlite

yashssh · 2024-06-07T08:51:05Z

Based on the changes introduced in #1042 by @modiking

yashssh · 2024-06-07T08:52:01Z

~~Please just review the last commit as it is rebased on top of #1046~~

Not relevant anymore

dlee992 · 2024-06-17T19:53:24Z

I "checked" the last commit. LGTM. only has some minor format issues, but I feel don't need to bring it up.
Also learned the difference of how to add a pass for old and new pass managers. Nice.

modiking · 2024-06-25T01:44:42Z

LGTM

yashssh · 2024-07-09T06:39:20Z

Rebased

yashssh · 2024-07-09T10:03:08Z

Clang format does not report anything for me locally. What version of clang-format is used in the checks?

@modiking

Based on the changes introduced in numba#1042 by @modiking

This reverts commit 2d9f8e6.

yashssh · 2024-07-18T11:17:28Z

~~I missed to update test_refprune.py and there might be some complications there. Consider this "not yet ready for review" for now.~~
Thanks :)

Edit: Ready for review

gmarkall · 2024-07-18T12:29:01Z

Clang format does not report anything for me locally. What version of clang-format is used in the checks?

It is clang-format-14, I think.

yashssh · 2024-07-19T08:16:29Z

using clang-format 14 made the tests pass, should llvmlite consider moving to a more recent clang-format?

yashssh · 2024-07-30T03:57:25Z

Gentle ping

yashssh

I have added some inline comments to make the thought process behind the the changes more apparent to the reviewers.

The gist of the changes are:

We prefix the old refprune passes with legacy keyword.
We create a new refprune pass following the new pass manager syntax.
Now that we have multiple instances of the same pass, to prevent duplication of code we move the computation functions to a more global scope(struct) so that both passes can share them.

Found this article online which comes very close to explaining what we did here https://www.neilhenning.dev/posts/llvm-new-pass-manager/

Examples from llvm codebase on porting a pass to NPM

yashssh · 2024-08-14T05:06:53Z

ffi/custom_passes.cpp

+void initializeRefNormalizeLegacyPassPass(PassRegistry &Registry);
+void initializeRefPruneLegacyPassPass(PassRegistry &Registry);


Prefix the pass name with "Legacy" to be used with legacy pass manager

yashssh · 2024-08-14T05:07:02Z

ffi/custom_passes.cpp

+class RefPrunePass : public PassInfoMixin<RefPrunePass> {
+
+  public:
+    Subpasses flags;
+    size_t subgraph_limit;
+    RefPrunePass(Subpasses flags = Subpasses::All, size_t subgraph_limit = -1)
+        : flags(flags), subgraph_limit(subgraph_limit) {}
+
+    PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM) {
+        auto &DT = AM.getResult<DominatorTreeAnalysis>(F);
+        auto &PDT = AM.getResult<PostDominatorTreeAnalysis>(F);
+        if (RefPrune(DT, PDT, flags, subgraph_limit).runOnFunction(F)) {
+            return PreservedAnalyses::none();
+        }
+
+        return PreservedAnalyses::all();
+    }
+};


Now that we have renamed the pass with "Legacy" keyword, we are free to create another pass with the name RefPrunePass, we will call this pass from NewPassManager . The passes for new pass manager follow this template mixin inheritance syntax that's visible here. There's also a difference in how we get the Analysis results needed for the pass to run, instead of explicitly specifying dependencies in getAnalysisUsage function we can directly query the NPM analysis manager for the result.

Note that wrapped in all this new pass manager syntax we are calling runOnFunction(F) which actually does the computation for this pass and is also used by legacy version of this pass.

yashssh · 2024-08-14T05:18:45Z

ffi/custom_passes.cpp

+struct RefNormalize {

-    bool runOnFunction(Function &F) override {
+    bool runOnFunction(Function &F) {


This is the core of the RefNormalize pass, we will call this function from legacy and new version of RefNormalize pass.

yashssh · 2024-08-14T05:36:27Z

ffi/custom_passes.cpp

+    All = PerBasicBlock | Diamond | Fanout | FanoutRaise
+} Subpasses;
+
+struct RefPrune {


We have moved all the computation related functions from inside struct RefPrunePass : public FunctionPass to struct RefPrune so that we can re-use these functions from both the Legacy pass and the new pass.

yashssh · 2024-08-14T05:39:46Z

ffi/custom_passes.cpp

+API_EXPORT(void)
+LLVMPY_AddRefPrunePass_module(LLVMModulePassManagerRef MPM, int subpasses,
+                              size_t subgraph_limit) {
+    llvm::unwrap(MPM)->addPass(
+        createModuleToFunctionPassAdaptor(RefNormalizePass()));
+    llvm::unwrap(MPM)->addPass(createModuleToFunctionPassAdaptor(
+        RefPrunePass((Subpasses)subpasses, subgraph_limit)));
+}
+
+API_EXPORT(void)
+LLVMPY_AddRefPrunePass_function(LLVMFunctionPassManagerRef FPM, int subpasses,
+                                size_t subgraph_limit) {
+    llvm::unwrap(FPM)->addPass(RefNormalizePass());
+    llvm::unwrap(FPM)->addPass(
+        RefPrunePass((Subpasses)subpasses, subgraph_limit));


API to register new passes in MPM and FPM respectively.

yashssh · 2024-08-14T05:43:22Z

llvmlite/binding/newpassmanagers.py

Copied the APIs from passmanagers.py

yashssh · 2024-08-14T05:46:23Z

llvmlite/tests/test_refprune.py

Creating new tests for refprune in new pass manager would have involved some duplication, so I migrated the old tests to NewPassManager. Let me know if you think that's not the best way forward.

gmarkall

Thanks for the PR - the code changes look good, and I was able to follow them well from the info you provided / linked to. I think there are two things that need adding:

Documentation for the add_refprune_pass functions of the new module and function pass managers
Keep the tests for the legacy pass manager, as well as testing with the new pass manager. Although there's a low probability of issues with the legacy pass manager, the changes do touch their code (and other future changes might potentially as well, we never know) so keeping both paths in use tested is important.

yashssh · 2024-08-20T06:19:50Z

Thanks for the review @gmarkall! I have addressed the comments.

gmarkall

Thanks for the additions - I think this is now good to merge!

esc · 2024-09-18T19:11:22Z

@yashssh @gmarkall code needs a fix, it's become conflicting with main due to recent merges. 🙏

yashssh · 2024-09-19T06:40:24Z

Resolved now, thanks!

esc · 2024-09-19T08:36:05Z

Resolved now, thanks!

@yashssh thank your for the swift update! -- @gmarkall can you check this and approve the fixes?

yashssh · 2024-09-24T05:54:35Z

Ping

gmarkall

The fixup looks good to me.

esc · 2024-09-24T14:50:52Z

Thank you for the patch!

esc requested a review from gmarkall June 7, 2024 10:09

esc added the 3 - Ready for Review label Jun 7, 2024

gmarkall mentioned this pull request Jun 11, 2024

Continuous Discussion: LLVM update plan #1048

Open

yashssh force-pushed the npm-refprune branch from de2a5ad to 4eeb079 Compare July 9, 2024 06:38

yashssh force-pushed the npm-refprune branch from 5000fd6 to 08639b5 Compare July 10, 2024 04:49

yashssh added 3 commits July 18, 2024 10:23

Port refprune to NewPassManager

6b21b2a

Based on the changes introduced in numba#1042 by @modiking

clang format

2d9f8e6

Revert "clang format"

4554776

This reverts commit 2d9f8e6.

yashssh force-pushed the npm-refprune branch from 08639b5 to 4554776 Compare July 18, 2024 04:56

yashssh added 2 commits July 19, 2024 12:19

Port refprune pass tests to npm

1e85a8b

clang-format 14 based formatting

2203726

yashssh mentioned this pull request Jul 30, 2024

Move Numba to LLVM's NewPassManager numba/numba#9676

Merged

yashssh commented Aug 14, 2024

View reviewed changes

gmarkall requested changes Aug 16, 2024

View reviewed changes

yashssh added 3 commits August 19, 2024 12:26

Doc for add_refprune_pass()

a619eb8

Add back tests for legacy version of refprune_pass

5e5b5c6

Merge branch 'main' into npm-refprune

13cb543

gmarkall previously approved these changes Aug 23, 2024

View reviewed changes

gmarkall added 5 - Ready to merge and removed 3 - Ready for Review labels Aug 23, 2024

esc added 4 - Waiting on author and removed 5 - Ready to merge labels Sep 18, 2024

Merge branch 'main' into npm-refprune

0251e46

yashssh dismissed gmarkall’s stale review via 0251e46 September 19, 2024 05:36

Fix corrupted test after merge

0b99743

gmarkall approved these changes Sep 24, 2024

View reviewed changes

gmarkall added 5 - Ready to merge and removed 4 - Waiting on author labels Sep 24, 2024

esc merged commit 23e02cb into numba:main Sep 24, 2024

stuartarchibald added this to the v0.44.0 milestone Nov 7, 2024

		void initializeRefNormalizeLegacyPassPass(PassRegistry &Registry);
		void initializeRefPruneLegacyPassPass(PassRegistry &Registry);

Conversation

yashssh commented Jun 7, 2024

Uh oh!

yashssh commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlee992 commented Jun 17, 2024

Uh oh!

modiking commented Jun 25, 2024

Uh oh!

yashssh commented Jul 9, 2024

Uh oh!

yashssh commented Jul 9, 2024

Uh oh!

yashssh commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmarkall commented Jul 18, 2024

Uh oh!

yashssh commented Jul 19, 2024

Uh oh!

yashssh commented Jul 30, 2024

Uh oh!

yashssh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yashssh Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

yashssh Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

yashssh Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

yashssh Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

yashssh Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

yashssh Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

yashssh Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmarkall left a comment

Choose a reason for hiding this comment

Uh oh!

yashssh commented Aug 20, 2024

Uh oh!

gmarkall left a comment

Choose a reason for hiding this comment

Uh oh!

esc commented Sep 18, 2024

Uh oh!

yashssh commented Sep 19, 2024

Uh oh!

esc commented Sep 19, 2024

Uh oh!

yashssh commented Sep 24, 2024

Uh oh!

gmarkall left a comment

Choose a reason for hiding this comment

Uh oh!

esc commented Sep 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yashssh commented Jun 7, 2024 •

edited

Loading

yashssh commented Jul 18, 2024 •

edited

Loading

yashssh left a comment •

edited

Loading

yashssh Aug 14, 2024 •

edited

Loading