Page MenuHomePhabricator

[FileCheck] Report missing prefixes when more than one is provided.
ClosedPublic

Authored by mtrofin on Oct 27 2020, 7:03 PM.

Details

Summary

If more than a prefix is provided - e.g. --check-prefixes=CHECK,FOO - we don't report if (say) FOO is never used. This may lead to a gap in our test coverage.

This patch introduces a new option, --allow-unused-prefixes. It currently is set to true, keeping today's behavior. After we explicitly set it in tests where this behavior was actually intentional, we will switch it to false by default.

http://lists.llvm.org/pipermail/llvm-dev/2020-October/146162.html

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2020, 7:03 PM
mtrofin requested review of this revision.Oct 27 2020, 7:03 PM

Please note: this patch isn't ready to land as-is. There are about 1500 tests failing, for the reason covered here. The goal of the patch is to start the discussion as to how we want to proceed - assuming we believe that we shouldn't have tests with prefixes that aren't exercised - so I suppose that's the first question.

Thanks!

So, this will break with the way the Attributor tests are designed. I will describe briefly how that is and why below. That said, I can see this to be useful, however, 1500 failures leads me to believe the Attributor tests are not the only ones that use such a pattern and it might be worth to have a way to keep the old behavior. Maybe we only error on tests that do not have auto generated check lines or we add a flag to FileCheck or something else ;)


Most Attributor tests have 4 run lines: {oldPM, newPM} x {module pass, cgscc pass}
Since we have 99 tests, 4 run lines each and a lot of things we want to check, we decided early on to use the update scripts.
Given all the enhancements we had to put in, some still downstream with me (incl. checking for global metadata), I think it is fair to say the Attributor was stress testing the update scripts quite a bit.
Anyway, to get the number of run lines down, and to make it explicit when the different combinations are equivalent or different, we use the 7 check prefixes per line, for example
CHECK,NOT_TUNIT_NPM,NOT_TUNIT_OPM,NOT_CGSCC_NPM,IS__CGSCC____,IS________OPM,IS__CGSCC_OPM
As we modify things we can simply run the update script and always find a valid set of check lines that is concise and expressive.
If we were to strip the unused prefixes (without having an automatic way to put them back) we would have substantially more work during the updates.
For us, the extra prefixes are not a problem or a bug. Whatever we do here, I would like a way to opt-in/out to keeping them around.

So, this will break with the way the Attributor tests are designed. I will describe briefly how that is and why below. That said, I can see this to be useful, however, 1500 failures leads me to believe the Attributor tests are not the only ones that use such a pattern and it might be worth to have a way to keep the old behavior. Maybe we only error on tests that do not have auto generated check lines or we add a flag to FileCheck or something else ;)


Most Attributor tests have 4 run lines: {oldPM, newPM} x {module pass, cgscc pass}
Since we have 99 tests, 4 run lines each and a lot of things we want to check, we decided early on to use the update scripts.
Given all the enhancements we had to put in, some still downstream with me (incl. checking for global metadata), I think it is fair to say the Attributor was stress testing the update scripts quite a bit.
Anyway, to get the number of run lines down, and to make it explicit when the different combinations are equivalent or different, we use the 7 check prefixes per line, for example
CHECK,NOT_TUNIT_NPM,NOT_TUNIT_OPM,NOT_CGSCC_NPM,IS__CGSCC____,IS________OPM,IS__CGSCC_OPM
As we modify things we can simply run the update script and always find a valid set of check lines that is concise and expressive.
If we were to strip the unused prefixes (without having an automatic way to put them back) we would have substantially more work during the updates.
For us, the extra prefixes are not a problem or a bug. Whatever we do here, I would like a way to opt-in/out to keeping them around.

To make sure I understand, is this capturing it: these test files are generated by a script. A test file will always list the exact same --check-prefixes list, but then internally will only use a subset of the prefixes list. Also for the sake of me understanding, an alternative would be to only generate the strict --check-prefixes subset that's used in the test file; but the added complexity here is that some functionality is still downstream.

If there were a (more terse) "--check-at-least-one-of-the-prefixes" with today's semantics, I assume the test generator produce tests using that option instead?

[...]
To make sure I understand, is this capturing it: these test files are generated by a script.

The IR code is provided by us, the check lines are generated by the llvm/utils/update_test_checks.py script.

A test file will always list the exact same --check-prefixes list, but then internally will only use a subset of the prefixes list.

Yes. Though the subset can change over time.

Also for the sake of me understanding, an alternative would be to only generate the strict --check-prefixes subset that's used in the test file; but the added complexity here is that some functionality is still downstream.

No. Downstream stuff is just to make the llvm/utils/update_test_checks.py better and allow more check lines to be generated. This is unrelated and I should not have mentioned it.

If we reduce it to a subset, it will work just fine for now. Then someone will change something and the test will break. At that point that person needs to either put the prefixes back, run the script, remove the unused prefixes again, *or* manually update the tests. The latter is not a great option for us.

If there were a (more terse) "--check-at-least-one-of-the-prefixes" with today's semantics, I assume the test generator produce tests using that option instead?

The run lines are written/copied by us. We could easily switch to a new option like that.

This strengthens D78024 (--implicit-check-not is specified) to the cases when --implicit-check-not is not used.
@jdenny had a use case similar to @jdoerfert https://reviews.llvm.org/D78024#inline-716591 (also CC @grimar and @probinson)

[...]
If there were a (more terse) "--check-at-least-one-of-the-prefixes" with today's semantics, I assume the test generator produce tests using that option instead?

The run lines are written/copied by us. We could easily switch to a new option like that.

I think we could add an option like --allow-unused-prefixes. It is probably sounds a bit simpler and can easily be added to generator I think.

grimar added inline comments.Oct 28 2020, 12:45 AM
llvm/lib/FileCheck/FileCheck.cpp
27

Seems unused?

1943

StringMap iteration order is not guaranteed to be deterministic. I think
some different container like MapVector probably should be used here instead.

+1 to the principle of this change - I wouldn't be surprised to see typos in at least some of the failing tests meaning that the incorrect check prefix isn't noticed. +1 also to an option to disable the check for the suggested reasons (I don't think I've personally got a use-case for it, but it seems like there are reasonable ones) - I'd be inclined to put the option in independently of the change that actually uses it, so that the tests can be updated independently, and gradually.

llvm/lib/FileCheck/FileCheck.cpp
1829

I'd prefer no auto here - what's the type of CP?

mtrofin updated this revision to Diff 301326.Oct 28 2020, 10:25 AM
mtrofin marked 2 inline comments as done.

using std::set for deterministic output and simpler code.

[...]
If there were a (more terse) "--check-at-least-one-of-the-prefixes" with today's semantics, I assume the test generator produce tests using that option instead?

The run lines are written/copied by us. We could easily switch to a new option like that.

I think we could add an option like --allow-unused-prefixes. It is probably sounds a bit simpler and can easily be added to generator I think.

That's a better name, indeed.

The remaining question is how to stage this. I think there are 2 broad ways to do it:

  1. we enable it by default, and this patch goes and patches all failing tests with an opt-out.
  2. we start with it disabled. Then, people opt in explicitly.

The problem I see with "2" is that there's no pressure to move to the "strict" place, which I think is what we want for a test suite.

The problem with "1" is 1500 failures. I think the way to mitigate that would be:

  • we start with the flag, and it does nothing
  • we annotate the tests (I can provide the list) - and this can be parallelized (i.e. perhaps folks can help with this)
  • we then land this patch

My preference is to go with nr "1".
What do folks think?

llvm/lib/FileCheck/FileCheck.cpp
1829

removed this.

1943

How about a std::set - it addresses determinism, the code is more readable (no "PrefixesNotFound.keys()" abstraction leaks), and any overhead it may have is likely not an issue, as we don't expect the prefixes list to be too large.

The remaining question is how to stage this. I think there are 2 broad ways to do it:

  1. we enable it by default, and this patch goes and patches all failing tests with an opt-out.
  2. we start with it disabled. Then, people opt in explicitly.

I think 1) is better but we need to address the 1500 failures. ~99 will go away when I patch the attributor tests, so I would suggest we first add the opt-out flag and start using it. Then we can keep track of problems in this commit and we don't have to touch 1500 files here. We should also look for patterns and write a script ;)

+1 to what other folks have said here, would be good to have a sample of the sort of tests that fail with this functionality - do we have lots of these either programmatically or otherwise symmetrically structured tests (some might be manually written but still benefit from a consistent pattern that may include unused check prefixes)? Or is it only a handful of places that would need to be fixed. But in any case, adding the flag upstream then committing changes incrementally to the tests or test-generators that need them, then eventually flipping the switch would probably be the way forward (unless we're talking like a huge % of all tests that need this flag added, then maybe it's not worthwhile/might need to look at more samples to discuss what the right tradeoffs/techniques are)

@mtrofin I favor a detect-potential-mistake-by-default approach as well. If you want to take the lead here, we can take a busy beavers approach. Add the option (initially true by default), create a spreadsheet with ~1500 failures grouped by test directories, post to llvm-dev and encourage maintainers to fix the issues (I would expect most are user errors, not deliberate --allow-unused-prefixes). After the issues are fixed, make the option false by default.

@mtrofin I favor a detect-potential-mistake-by-default approach as well. If you want to take the lead here, we can take a busy beavers approach. Add the option (initially true by default), create a spreadsheet with ~1500 failures grouped by test directories, post to llvm-dev and encourage maintainers to fix the issues (I would expect most are user errors, not deliberate --allow-unused-prefixes). After the issues are fixed, make the option false by default.

Awesome, let's do that! The list is actually captured here - the failed unittests - but we can do a shared spreadsheet and go that way.

Let me add the flag first (need a few mins, should have it in by lunch)

@mtrofin I favor a detect-potential-mistake-by-default approach as well. If you want to take the lead here, we can take a busy beavers approach. Add the option (initially true by default), create a spreadsheet with ~1500 failures grouped by test directories, post to llvm-dev and encourage maintainers to fix the issues (I would expect most are user errors, not deliberate --allow-unused-prefixes). After the issues are fixed, make the option false by default.

Awesome, let's do that! The list is actually captured here - the failed unittests - but we can do a shared spreadsheet and go that way.

Let me add the flag first (need a few mins, should have it in by lunch)

Oh - I was going to say, let's add the flag, then wait like a day or so, if bulk sets of tests can be handled that way by their authors (e.g. @jdoerfert 's scenario).

MaskRay added a comment.EditedOct 28 2020, 12:38 PM

You can add the option in this patch and create a RFC on llvm-dev (I think people have favored the idea in the past but I cannot recall the thread now). Maintainers do one of the two:

  • Add --allow-unused-prefixes if their use cases are similar to @jdoerfert and @jdenny's
  • Fix the test prefixes (usually due to stale RUN lines after some check prefixes related changes). I think the majority of ~1500 tests belongs to this category.

This can be a collaborative spreadsheet so that people can claim the work.

mtrofin updated this revision to Diff 301500.Oct 28 2020, 6:20 PM

included the opt/in/out flag, and a test.

Also fixed one test that (from what I read) doesn't actually need FileCheck

mtrofin edited the summary of this revision. (Show Details)Oct 28 2020, 6:22 PM

You can add the option in this patch and create a RFC on llvm-dev (I think people have favored the idea in the past but I cannot recall the thread now). Maintainers do one of the two:

  • Add --allow-unused-prefixes if their use cases are similar to @jdoerfert and @jdenny's
  • Fix the test prefixes (usually due to stale RUN lines after some check prefixes related changes). I think the majority of ~1500 tests belongs to this category.

This can be a collaborative spreadsheet so that people can claim the work.

SGTM - I'll post a RFC tomorrow, with list. This will also give a chance to folks in other time zones to chime in, if there are some additional considerations (less churn to the RFC, if that is the case)

grimar added inline comments.Oct 28 2020, 11:39 PM
llvm/test/FileCheck/allow-unused-prefixes.txt
2

You probably need to test the error reported?

3

I think it also worth to add a test for the default value, i.e. when no --allow-unused-prefixes is present.

11

No EOL.

Please also update the FileCheck documentation with the new option.

llvm/lib/FileCheck/FileCheck.cpp
1941–1942

If I follow this correctly, let's say I have specified --check-prefixes=FOO,BAR, and then only use FOO, I'll get the message error: no check strings found with prefixes 'BAR', but really it should be with prefix 'BAR' (i.e. no "es"). The logic here probably wants to be using PrefixesNotFound.size(), right?

llvm/test/FileCheck/allow-unused-prefixes.txt
3
5

I don't think you need this line, because the P1 line will match itself (by default FileCheck doesn't require full line matches).

6

Be consistent with your "comment" markers - you've used both ';' and '//' in this file. They're actually not needed here at all, but are probably useful to help the RUN/CHECK lines stand out from the rest of the data.

llvm/test/Transforms/NewGVN/todo-pr37121-seens-this-value-a-lot.ll
3 ↗(On Diff #301500)

Seems unrelated?

mtrofin updated this revision to Diff 301640.Oct 29 2020, 8:45 AM
mtrofin marked 8 inline comments as done.

incorporated feedback

mtrofin updated this revision to Diff 301643.Oct 29 2020, 8:50 AM
mtrofin marked an inline comment as done.

removed unrelated test change

mtrofin added inline comments.Oct 29 2020, 10:11 AM
llvm/lib/FileCheck/FileCheck.cpp
1941–1942

Yup - fixed.

llvm/test/FileCheck/allow-unused-prefixes.txt
2

Added for both single and multiple missing prefixes. Note that, because the way prefixes are reported, it required a separation of the test input and this verification. (otherwise <comment> .* Prefix: would match to a prefix)

5

--allow-empty is false by default

Gentle reminder - is this good to go?

mtrofin edited the summary of this revision. (Show Details)Oct 30 2020, 7:01 AM
mtrofin edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Oct 30 2020, 7:09 AM
This revision was landed with ongoing or failed builds.Oct 30 2020, 12:39 PM
This revision was automatically updated to reflect the committed changes.

Sorry, got a bit eager submitting, as I wanted to unblock uses of the new flag. Happy to address post-submit feedback, if any!

The documentation wasn't updated. Please do that.

llvm/test/FileCheck/Inputs/one-check.txt
3

Needs new line at EOF.

llvm/test/FileCheck/allow-unused-prefixes.txt
1

Should this test be using %ProtectFileCheckOutput?

5–6

I try to encourage using double comment markers for real comments as opposed to lit directive markers, e.g. ;; here.

11

This comment wasn't addressed.

mtrofin marked 4 inline comments as done.Nov 2 2020, 8:54 AM

The documentation wasn't updated. Please do that.

Sorry I missed that - please see https://reviews.llvm.org/D90621

llvm/test/FileCheck/allow-unused-prefixes.txt
1

Not sure why, we're not setting env variables, or am I missing something?

dblaikie added inline comments.Nov 2 2020, 9:49 AM
llvm/test/FileCheck/allow-unused-prefixes.txt
1

The test isn't setting environment variables, but users can (see: https://reviews.llvm.org/D65121 ) to configure FileCheck's behavior for their convenience/to produce more actionable errors (I think this is to allow customization both by users, but also on buildbots so buildbots fail with more context that makes it easier to debug from the logs when you can't necessarily reproduce the command locally). So any test that relies on the output of FileCheck probably wants to ensure FILECHECK_OPTS can't effect the output of the FileCheck command under test and can/should do so using %ProtectFileCheckOutput

mtrofin marked an inline comment as done.Nov 2 2020, 10:20 AM
mtrofin added inline comments.
llvm/test/FileCheck/allow-unused-prefixes.txt
1

Thanks for the explanation, that is the bit that I couldn't grok from %ProtectFileCheckOutput, somehow I read "developers" as "developers writing tests and setting variables in their tests' RUN line".

mtrofin marked an inline comment as done.Nov 2 2020, 10:28 AM
mtrofin added inline comments.
llvm/test/FileCheck/allow-unused-prefixes.txt
1

@mtrofin llvm/test/Analysis now passes with -allow-unused-prefixes=false - is there a way that we can use lit.local.cfg to keep it clean?

lit.local.cfg would "blanket-allow" future tests, too. Would that be
desirable?

lit.local.cfg would "blanket-allow" future tests, too. Would that be desirable?

Yes I'm finding that 99% of llvm tests that were failing were just poor check-prefix setup (usually copy+paste or too much emphasis on an unnecessarily complex prefix hierarchy) - so until you can flip the default value in FileCheck itself we should be able to use lit.local.cfg files in the subfolder root to keep them clean once they've been fixed up.

Sorry, I misread your original email: you're saying: use lit.local.cfg to
blanket-set the "don't allow unused prefixes" behavior on conforming
directories (I read it backwards).

I *think* the main challenge is that the -allow-unused-prefixes flag must
occur only once?