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

[StandardInstrumentations] Support -print-after-pass-number option #87458

Merged
merged 6 commits into from
Apr 11, 2024

Conversation

thevinster
Copy link
Contributor

There's already support for -print-before-pass-number, so it makes sense that we also have a -print-after-pass-number. This is especially useful if you want to print the IR after the very last pass without resorting to -print-after-all and combing through stderr or the IR file directory.

Copy link

github-actions bot commented Apr 3, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@@ -802,6 +807,8 @@ void PrintIRInstrumentation::printBeforePass(StringRef PassID, Any IR) {
(shouldPrintBeforePass(PassID) || shouldPrintAfterPass(PassID)))
DumpIRFilename = fetchDumpFilename(PassID, IR);

++CurrentPassNumber;
Copy link
Contributor

Choose a reason for hiding this comment

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

this will make pass numbers non-consecutive when we have passes we don't print IR for. is this change necessary? can you find a way to preserve the old behavior? (we should probably add a test that the pass numbers are consecutive)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

print-at-pass-number.ll does test the pass numbers are consecutive (although it doesn't test passes where the IR isn't printed). If we want to preserve the existing behavior, I can hoist the shouldPrintIR call above. The reason for hoisting incrementing CurrentPassNumber is so to have the shouldPrintAfterPass run after which is needed in the printAfterPass call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the CurrentPassNumber back to where it was to preserve the behavior of consecutive pass numbers for printing filtered IR. I also added a test for it in the same file. The new solution is to push a new PassRunDescriptor after CurrentPassNumber has been incremented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh nevermind, this approach doesn't work as it doesn't clean the PassRunDescriptor stack up. The new approach is to implement a separate check that only pushes the PassRunDescriptor when the you are running with -print-after-pass-number option.

@thevinster
Copy link
Contributor Author

Ping. @aeubanks I think this version should satisfy your requirements.

@@ -929,6 +942,11 @@ bool PrintIRInstrumentation::shouldPrintAfterPass(StringRef PassID) {
return is_contained(printAfterPasses(), PassName);
}

bool PrintIRInstrumentation::shouldPrintAfterPassNumberCheck() {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldPrintAfterPassNumberCheck/shouldPrintAfterPassNumber is a bit confusing. perhaps shouldPrintAfter/BeforePassNumber should be shouldPrintAfter/BeforeSomePassNumber, and shouldPrintAfter/BeforePassNumberCheck should be shouldPrintAfter/BeforeCurrentPassNumber?

…ber and shouldPrintAfter/BeforePassNumberCheck->shouldPrintAfter/BeforeCurrentPassNumber
@thevinster thevinster merged commit 03f619d into llvm:main Apr 11, 2024
4 checks passed
@thevinster thevinster deleted the print-after-pass-number branch April 12, 2024 23:03
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