search

numba / llvmlite

A lightweight LLVM python binding for writing JIT compilers
https://llvmlite.pydata.org/
BSD 2-Clause "Simplified" License
1.94k stars 322 forks source link

Add instrumentation callback hook for new pass managers #1069

Closed yashssh closed 4 months ago

yashssh commented 4 months ago

The new pass manager relies on PIC callbacks for deciding if a function needs to be skipped for optimization when compiled with 'optnone'.

esc commented 4 months ago

Looks like the C++ is failing to compile in this PR at present.

yashssh commented 4 months ago

It's a llvm16 specific failure, I can see it offline as well. Looking into it

yashssh commented 4 months ago

Thanks for the review @gmarkall!

we create a PassBuilder in order to get a ModulePassManager, but for running the module pass manager, we end up ignoring the PassBuilder we did have and creating a new one

Yes that looks a bit weird and somewhat hacky, I did that because PassBuilder's constructor takes PIC as one of the argument after we have registered the callbacks. And for registering call backs we need the analysis manager's object, it was creating a "cyclic dependency" type situation, to fix it I tried to just use a new PassBuilder object which is exactly identical to the old pass builder object.

My gut feeling is that we should be exposing the PassInstrumentationCallbacks and PrintPassOptions as Python classes too, which would allow us to construct the PassBuilder in the way that we require

Yes, I agree. That would be a better way to implement this. I was concerned that we would end up creating too many classes in the python code and it might be better to just hide all the messy stuff in the back-end.

I'll update the code to reflect these changes.

yashssh commented 4 months ago

I have no problem with PIC being exposed in Python, but I wonder if it wasn't actually necessary because we create the PassBuilder with the PIC, and presumably the PassBuilder can give a reference to that PIC at any time we need it anyway?

Yes, you are right. I have updated the code.

yashssh commented 4 months ago

Does this need rebasing?

esc commented 4 months ago

@yashssh I am not sure, we tried to merge this, but it seems like our approvals keep getting dismissed automatically? I've never seen this before. Do you have any idea what might be going on?

esc commented 4 months ago

Is this a GitHub bug?

https://github.com/orgs/community/discussions/58535

yashssh commented 4 months ago

Closing and reopening seems to have worked for some, I can try that?

esc commented 4 months ago

Closing and reopening seems to have worked for some, I can try that?

by all means, yes please

gmarkall commented 4 months ago

This sounds like the potential scenario that triggered it here: https://github.com/orgs/community/discussions/58535#discussioncomment-6495938 - I think this started based on #1068?

yashssh commented 4 months ago

I think this started based on #1068?

Yes

yashssh commented 4 months ago

I can see all the commits that were part of the PR in the commit history on main branch, is it common practice?

esc commented 4 months ago

I can see all the commits that were part of the PR in the commit history on main branch, is it common practice?

In git there is no such concept as a commit being "on a branch". The commit history is a DAG and branches are pointers at specific nodes in the graph. When a PR is merged to main all the commits of the PR are reachable from the main branch, that's the whole point. Makes sense?

yashssh commented 4 months ago

Ahh yes.. thank you!

From: Emergency Self-Construct @.> Sent: Wednesday, July 24, 2024 5:02:27 PM To: numba/llvmlite @.> Cc: Yashwant Singh @.>; State change @.> Subject: Re: [numba/llvmlite] Add instrumentation callback hook for new pass managers (PR #1069)

I can see all the commits that were part of the PR in the commit history on main branch, is it common practice?

In git there is no such concept as a commit being "on a branch". The commit history is a DAG and branches are pointers at specific nodes in the graph. When a PR is merged to main all the commits of the PR are reachable from the main branch, that's the whole point. Makes sense?

— Reply to this email directly, view it on GitHubhttps://github.com/numba/llvmlite/pull/1069#issuecomment-2247676085, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A3GNWZ7Z5OQQ4JMSFJKMED3ZN6GEXAVCNFSM6AAAAABLB2MLV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBXGY3TMMBYGU. You are receiving this because you modified the open/close state.Message ID: @.***>

esc commented 4 months ago

Ahh yes.. thank you!

Here, perhaps this can aid comprehension: https://fossy-cats.github.io/Git-Buch_EN/Git-Book_4.html (it's an english translation of a git book I wrote more than a decade ago -- the concepts are still very relevant today.)

yashssh commented 4 months ago

Here, perhaps this can aid comprehension: https://fossy-cats.github.io/Git-Buch_EN/Git-Book_4.html (it's an english translation of a git book I wrote more than a decade ago -- the concepts are still very relevant today.)

Will give it a read, thanks again!