Skip to content

[NewPM][BPF] Add BPFCodeGenPassBuilder #94158

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

paperchalice
Copy link
Contributor

We need a simple enough target to make codegen pipeline short, so we can handle important passes (e.g. asm printer) as soon as possible.

Copy link
Member

@inclyc inclyc left a comment

Choose a reason for hiding this comment

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

Just a nit picking.

Is this an adoption to refactored NewPM pass builder?

#89708

We need a simple enough target to make codegen pipeline as short as possible.
@paperchalice
Copy link
Contributor Author

paperchalice commented Jun 3, 2024

Just a nit picking.

Is this an adoption to refactored NewPM pass builder?

#89708

No, how to extend the codegen pipeline is still controversial. It seems that using callback functions is better than polymorphic member function.

Copy link
Member

@4ast 4ast left a comment

Choose a reason for hiding this comment

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

Code churn and nothing more. No.

@aeubanks
Copy link
Contributor

aeubanks commented Jun 3, 2024

Code churn and nothing more. No.

can you clarify your comment? we're trying to make codegen work with the new pass manager

aeubanks
aeubanks previously approved these changes Jun 17, 2024
Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

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

anyway, lgtm

@4ast
Copy link
Member

4ast commented Jun 17, 2024

Code churn and nothing more. No.

can you clarify your comment? we're trying to make codegen work with the new pass manager

all TODO are screaming that this is useless change.
Why do you want empty callbacks with TODO comments?

@4ast 4ast dismissed aeubanks’s stale review June 17, 2024 19:27

still no.

//
//===----------------------------------------------------------------------===//
//
// This file implements BPFCodeGenPassBuilder class.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove this comment, it's not very useful

@aeubanks
Copy link
Contributor

Code churn and nothing more. No.

can you clarify your comment? we're trying to make codegen work with the new pass manager

all TODO are screaming that this is useless change. Why do you want empty callbacks with TODO comments?

because we're going to implement them in a followup change

this PR is just boilerplate and doesn't actually do much. however, it makes follow-up changes smaller which is valuable for reviewing

@4ast
Copy link
Member

4ast commented Jun 17, 2024

this PR is just boilerplate and doesn't actually do much. however, it makes follow-up changes smaller which is valuable for reviewing

Not really. At present this is pointless code churn and nothing else. Make a meaningful diff and describe the reasons to make it.

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

Successfully merging this pull request may close these issues.

4 participants