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

[Pass] Add some missing passes #77600

Closed
wants to merge 1 commit into from
Closed

Conversation

paperchalice
Copy link
Contributor

@paperchalice paperchalice commented Jan 10, 2024

When dumping IR pipeline in unit test of PR #77084, these passes show their class names, so add them to pass builder to ensure all names are populated.
Also, when invoke opt -print-pipeline-passes without -S, it will print:
Could not parse dumped pass pipeline: unknown module pass 'BitcodeWriterPass'

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

Can you update the description of the PR to better describe the motivation and context of the patch so that the commit message is more useful once this patch lands?

llvm/lib/Passes/PassRegistry.def Outdated Show resolved Hide resolved
@@ -142,6 +143,7 @@ MODULE_PASS("tsan-module", ModuleThreadSanitizerPass())
MODULE_PASS("verify", VerifierPass())
MODULE_PASS("view-callgraph", CallGraphViewerPass())
MODULE_PASS("wholeprogramdevirt", WholeProgramDevirtPass())
MODULE_PASS("write-bitcode", BitcodeWriterPass(nulls(), true))
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of including this here? The canonical way to use the bitcode writer pass seems to be to add it manually to the pipeline so whatever is using the pipeline can set it's own output stream (which is just set to null here, seeming to make the pass not really useful?).

Copy link
Contributor Author

@paperchalice paperchalice Jan 11, 2024

Choose a reason for hiding this comment

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

Yes, but when using opt -print-pipeline-passes foo.ll will report:

Could not parse dumped pass pipeline: unknown module pass 'BitcodeWriterPass'

Or we can manually add this pass name in constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add BitcodeWriterPass manually in constructor now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that error is from running the opt invocation with the textual representation of the pass pipeline from -print-pipeline-passes? What is your invocation to get the Bitcode Writer in the printed pass pipeline? That doesn't occur (for me) with ToT opt with something like opt -passes='default<O3>' -print-pipeline-passes test.ll for example.

Copy link
Contributor Author

@paperchalice paperchalice Jan 11, 2024

Choose a reason for hiding this comment

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

Oh, I forget the -o option. opt -passes='default<O3>' -print-pipeline-passes test.ll -o test.bc will print the error.

Copy link
Contributor

@boomanaiden154 boomanaiden154 Jan 11, 2024

Choose a reason for hiding this comment

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

Ah. I should've figured -disable-output on my invocation would never create the bitcode writer. I'm not convinced the current approach (currently manually calling addClassToPassName in the PassBuilder constructor) is best. Others probably have better ideas on the best approach to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test new-pm-print-pipeline.ll failed. It seems like this was done on purpose, will revert related change.

When dumping IR pipeline in unit test, these passes show their class names,
so add them to pass builder to ensure all names are populated.
Also, when invoke `opt -print-pipeline-passes` without `-S`, it will print:
Could not parse dumped pass pipeline: unknown module pass 'BitcodeWriterPass'
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.

None yet

2 participants