Skip to content

[BOLT] Increase nfc-test coverage #467

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 2 commits into
base: users/paschalis-mpeis/set-nfc-tests-once
Choose a base branch
from

Conversation

paschalis-mpeis
Copy link
Member

@paschalis-mpeis paschalis-mpeis commented Jun 9, 2025

Use --check-bolt-sources from nfc-check-setup.py to run tests when source
files change between the current and previous revisions.

Skip markers are used to indicate which tests should run: when not present the
tests run. Examples:

  • If NFC check setup fails to execute for any reason, the tests run unconditionally.
  • If the current and previous llvm-bolt binaries are identical, we create two
    skip markers for in-tree and out-of-tree tests.
  • If there are other BOLT source changes, we delete the in-tree skip marker so
    those test can proceed.

@paschalis-mpeis
Copy link
Member Author

paschalis-mpeis commented Jun 10, 2025

This PR triggers in-tree tests for additional source changes. Running out-of-tree tests is not needed.

The main motivation is to cover patches #1, #2, and #3 (see table below). It also catches other cases, like driver-file modifications (e.g., breaking heatmap.cpp without altering llvm-bolt).

Once merged, any changes to nfc-check-setup.py itself will automatically re-trigger tests, as an extra sanity check to the configuration.

I've verified these internally and got expected behaviour in the below patches:

# PATCH CODE CHANGE In-Tree Out-Of-Tree
1 e24d86 Modifies only a BOLT test. Draft Draft
2 6090c0 Modifies only a BOLT test. Draft Draft
3 686ec6 Modifies only a BOLT test. Draft Draft
4 b037d0 Modifies bolt heatmap docs. Draft Draft
5 7a688c Modifies LLVM: an unsupported backend (ARM). Draft Draft
6 3a9893 Modifies LLVM: a supported backend (AArch64). Draft Draft

@paschalis-mpeis paschalis-mpeis marked this pull request as ready for review June 10, 2025 15:39
@paschalis-mpeis paschalis-mpeis requested a review from aaupov June 10, 2025 15:39
@paschalis-mpeis paschalis-mpeis force-pushed the users/paschalis-mpeis/set-nfc-tests-once branch from b03cc23 to 9ffb74e Compare June 13, 2025 09:48
@paschalis-mpeis paschalis-mpeis force-pushed the users/paschalis-mpeis/increase-test-coverage branch from 8f4aaf1 to 326abf3 Compare June 13, 2025 09:57
@paschalis-mpeis
Copy link
Member Author

Forced pushed to rebase. Also now would run the tests unconditionally, if for any reason the nfc-check-setup.py had failed.

Copy link
Contributor

@aaupov aaupov left a comment

Choose a reason for hiding this comment

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

Thanks

@paschalis-mpeis paschalis-mpeis force-pushed the users/paschalis-mpeis/set-nfc-tests-once branch from 9ffb74e to 154a315 Compare June 18, 2025 09:49
paschalis-mpeis and others added 2 commits June 18, 2025 10:59
Use `--check-bolt-sources` from nfc-check-setup.py to run tests
when source files change between the current and previous revisions.

This triggers only in-tree tests. When the llvm-bolt binary changes,
both in-tree and out-of-tree tests are run.
If the NFC check setup fails to execute for any reason, we still want to run
tests unconditionally. To implement this, we now use skip markers: tests run
when these markers are not present.

If the current and previous llvm-bolt binaries are identical, we create two
skip markers for in-tree and out-of-tree tests. If there are other BOLT source
changes, we delete the in-tree skip marker so those test Can proceed.
@paschalis-mpeis paschalis-mpeis force-pushed the users/paschalis-mpeis/increase-test-coverage branch from 68cc918 to a16e2be Compare June 18, 2025 10:00
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.

2 participants