[llvm-dev] [FileCheck] Error for unused check prefixes (was: [RFC] FileCheck: (dis)allowing unused prefixes)

[Fāng-ruì Sòng via llvm-dev]

Last October Mircea sent an RFC about (dis)allowing unused check prefixes in FileCheck: https://lists.llvm.org/pipermail/llvm-dev/2020-October/146162.html

(In short, if FileCheck --check-prefix or --check-prefixes specifies a prefix which does not occur in the test, FileCheck will error. Note: a `-NOT` pattern is also counted as an occurrence.)

Mircea created a worksheet https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFbTDzC_S09e973_cwYuw/edit#gid=0 which may be a bit stale now but llvm-project has reached a state where all issues have been identified and fixed, or worked around (by opting in FileCheck --allow-unused-prefixes in some test directories, such as: clang/test/OpenMP, llvm/test/Transforms/Attributor and llvm/test/FileCheck).

We can make a switch if the community thinks that not allowing unused prefixes is the better default: https://reviews.llvm.org/D95849

For downstream projects using FileCheck and lit, it should be easy to restore the previous permissive behavior

from lit.llvm.subst import ToolSubst

fc = ToolSubst('FileCheck', unresolved='fatal')
config.substitutions.insert(0, (fc.regex, 'FileCheck --allow-unused-prefixes'))

# Note: if multiple --allow-unused-prefixes options are specified, the last wins.

Personally I am strongly in favor of disallowing unused check prefixes by default. We have identified numerous issues:

(1) typo. A misspelled check prefix does not test what it intends to test.

(2) bitrot tests due to refactoring

(3) unspecified `-NOT` patterns. Sometimes a test uses something like `--check-prefixes=COMMON,TRUE` and `--check-prefixes=COMMON,FALSE` to test both behaviors but they forget to include a `FALSE-NO:` pattern to test that some string does not appear.

(1) and (2) are especially common. There are indeed tests where --allow-unused-prefixes is more suitable - but they are sufficiently few that I think the default should be --allow-unused-prefixes=false.

So, what do folks think?

On Mon, Nov 16, 2020 at 8:22 AM Mircea Trofin <mtr...@google.com> wrote:

Thanks for doing this (as well as the awesome cleanup in CodeGen/X86)!

On Sun, Nov 15, 2020 at 6:31 PM Wang, Pengfei <pengfe...@intel.com> wrote:

Hi,

I wrote a script to help removing unused trivial prefixes. It is located in llvm/utils/update_test_prefix.py.
It is a rough one and based on problems I met. It also highly relies on the output of llvm-lit and update tools. So carefully verification on its changes is MUST.

Here's the usage of it:
1. turn allow-unused-prefixes to false and built it: sed -i '/allow-unused-prefixes/s/true/false/' llvm/utils/FileCheck/FileCheck.cpp;
2. cd llvm-project;
3. make sure llvm-lit and update_llc_test_checks.py or other update tools are available;
4. llvm/utils/update_test_prefix.py test_file_you_want_to_update.xxx
5. you can also put them in a file and cat update_tests.txt | xargs llvm/utils/update_test_prefix.py

The script removes unused prefixes and verifies those autogenerated tests. It reports "Failed" if there's conflicting in regeneration and "Changed" if there're changes in the check contents. There's a known issue if the prefix is not connected by "=", e.g. "--check-prefix X86". It will be put into "Aborted". Please pay attention for tests in these lists.
Wish the script would help you😊

Thanks
Pengfei

-----Original Message-----
From: Fāng-ruì Sòng <mas...@google.com>
Sent: Friday, November 6, 2020 3:50 AM
To: Matt Arsenault <Matthew....@amd.com>; Alexey...@ibm.com; Tim Northover <t.p.no...@gmail.com>; Wang, Pengfei <pengfe...@intel.com>
Cc: Mircea Trofin <mtr...@google.com>; David Blaikie <dbla...@gmail.com>; LLVM Dev <llvm...@lists.llvm.org>
Subject: Re: [llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes

On Thu, Nov 5, 2020 at 11:36 AM David Blaikie via llvm-dev <llvm...@lists.llvm.org> wrote:
>
>
>
> On Thu, Nov 5, 2020 at 10:46 AM Mircea Trofin <mtr...@google.com> wrote:
>>
>>
>>
>> On Thu, Nov 5, 2020 at 10:40 AM David Blaikie <dbla...@gmail.com> wrote:
>>>
>>>
>>>
>>> On Thu, Nov 5, 2020 at 7:30 AM Mircea Trofin via llvm-dev <llvm...@lists.llvm.org> wrote:
>>>>
>>>> There are currently 1350 owner-less failures in the spreadsheet. These seem to be the larger areas there.
>>>>
>>>> If you see an area you have ownership or expertise in, please sign up for fixing the tests by Monday, Nov. 9.
>>>>
>>>> Otherwise, I will "blanket-add" --allow-unused-prefixes=true to the remaining failing tests.
>>>
>>>
>>> If/when you do that, probably worth adding a comment at each site to clarify that this was added automatically, not vetted/intentionally added by a human. Something like "// FIXME: Verify that unused prefixes are used intentionally" or the like.
>>
>> Ack. or, we can grep for -allow-unused-prefixes=true, wdyt?
>
>
> Not sure I understand who/when they would grep for that?
>
> I was suggesting adding an explicit "this use of --allow-unused-prefixes=true hasn't been confirmed as intentional" so that the backwards compatibility cases can be distinguished from the intentional cases when someone is reading the test case, rather than puzzling over why this flag was added (which looks intentional) though the unused prefix may not make sense in that particular test. It'll make it easier in the future when someone does look at the test for them to not feel like they're being implicitly told "this use of unused prefixes is intentional" (by the presence of an explicit flag requesting such support) while staring at the test and not being able to see why someone would've done that intentionally.

Directly CCing some folks who can fix or find people to fix some directories (*/AMDGPU, */X86, */OpenMP, */AArch64) on
https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFbTDzC_S09e973_cwYuw/edit#gid=0
After these big directories are cleaned up, the remaining tests should be manageable in amount.

How to reproduce:

sed -i '/allow-unused-prefixes/s/true/false/' llvm/utils/FileCheck/FileCheck.cpp
git update-index --assume-unchanged llvm/utils/FileCheck/FileCheck.cpp
ninja check-llvm   # or check-clang ...




>>>
>>>
>>>>
>>>>
>>>>
>>>> On Fri, Oct 30, 2020 at 12:48 PM Mircea Trofin <mtr...@google.com> wrote:
>>>>>
>>>>> An update: as of 871d658c9ceb, the flag is now available, if folks need to use it.
>>>>>
>>>>> On Thu, Oct 29, 2020 at 10:28 AM Mircea Trofin <mtr...@google.com> wrote:
>>>>>>
>>>>>> Hello all,
>>>>>>
>>>>>> TL;DR; if you used FileCheck --check-prefixes and you missed (misspelled, for instance) one of the prefixes in your test, FileCheck silently ignores that and the test passes.
>>>>>>
>>>>>> 1579 tests have this property.
>>>>>>
>>>>>>
>>>>>> The details
>>>>>> =========
>>>>>> Please refer to https://reviews.llvm.org/D90281 and the
>>>>>> discussion there for more details (make sure you open "older
>>>>>> changes" for full context)
>>>>>>
>>>>>> The problem is covered by the TL;DR;.
>>>>>>
>>>>>> The proposal is to add an explicit flag to FileCheck, --allow-unused-prefixes, to indicate whether the current behavior is intended (for instance, jdoerfert contributed a scenario where that is the case).
>>>>>>
>>>>>> We want the default behavior to be 'strict', i.e. --allow-unused-prefixes=false. Doing that right now would lead to 1500 test failures.
>>>>>>
>>>>>> To get there (thanks, maskray, for suggestion), we propose we:
>>>>>> * land D90281 where the flag is introduced, but is flipped to
>>>>>> match today's behavior
>>>>>> * employ a 'busy beavers' approach, where test maintainers patch their tests:
>>>>>>   - either leveraging the flag, to explicitly indicate that unused prefixes is intended (i.e. add --allow-unused-patches=true); or
>>>>>>   - fix the test (e.g. maybe there was a misspelling issue/omission/etc).
>>>>>>
>>>>>> A spreadsheet with the failing tests is available here [1].
>>>>>>
>>>>>> The request to the community members is to please sign up for their respective area in the spreadsheet, and then mark it completed when that's the case (yes/no in the respective column).
>>>>>>
>>>>>> When all the tests are fixed, we will then flip --allow-unused-prefixes to false by default.
>>>>>>
>>>>>> Meanwhile, please consider leveraging the flag explicitly when you author new tests that use --check-prefixes. That can be then cleaned up easily after we switch to the 'strict' behavior.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> [1]
>>>>>> https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFb
>>>>>> TDzC_S09e973_cwYuw/edit?usp=sharing
>>>>
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm...@lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev



--
宋方睿

--

宋方睿

[James Henderson via llvm-dev]

In my opinion, the cost of a bit of inconvenience for users who want to allow unused prefixes is far outweighed by the potential to catch bugs (both test bugs and more importantly genuine bugs hidden by test bugs), so +1 to the default being false for the option.

[via llvm-dev]

Agreed, in particular this half of the catching-typos problem is definitely worth solving.

(The other half would be typos in the CHECK directives themselves, which is a distinctly harder problem and one that FileCheck cannot solve on its own.)

--paulr

[Nikita Popov via llvm-dev]

On Wed, Feb 3, 2021 at 12:50 AM Fāng-ruì Sòng <mas...@google.com> wrote:

Last October Mircea sent an RFC about (dis)allowing unused check prefixes in FileCheck: https://lists.llvm.org/pipermail/llvm-dev/2020-October/146162.html

(In short, if FileCheck --check-prefix or --check-prefixes specifies a prefix which does not occur in the test, FileCheck will error. Note: a `-NOT` pattern is also counted as an occurrence.)

Mircea created a worksheet https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFbTDzC_S09e973_cwYuw/edit#gid=0 which may be a bit stale now but llvm-project has reached a state where all issues have been identified and fixed, or worked around (by opting in FileCheck --allow-unused-prefixes in some test directories, such as: clang/test/OpenMP, llvm/test/Transforms/Attributor and llvm/test/FileCheck).

We can make a switch if the community thinks that not allowing unused prefixes is the better default: https://reviews.llvm.org/D95849

For downstream projects using FileCheck and lit, it should be easy to restore the previous permissive behavior

from lit.llvm.subst import ToolSubst

fc = ToolSubst('FileCheck', unresolved='fatal')
config.substitutions.insert(0, (fc.regex, 'FileCheck --allow-unused-prefixes'))

# Note: if multiple --allow-unused-prefixes options are specified, the last wins.

Personally I am strongly in favor of disallowing unused check prefixes by default. We have identified numerous issues:

(1) typo. A misspelled check prefix does not test what it intends to test.

(2) bitrot tests due to refactoring

(3) unspecified `-NOT` patterns. Sometimes a test uses something like `--check-prefixes=COMMON,TRUE` and `--check-prefixes=COMMON,FALSE` to test both behaviors but they forget to include a `FALSE-NO:` pattern to test that some string does not appear.

(1) and (2) are especially common. There are indeed tests where --allow-unused-prefixes is more suitable - but they are sufficiently few that I think the default should be --allow-unused-prefixes=false.

So, what do folks think? 

Just my 2c, but I think we should allow unused prefixes. This does catch the occasional typo, but also has a cost: Historically, certain kinds of tests simply used a certain boilerplate of check lines, because differences are common, even if they don't occur for each test. For X86 vector tests, it makes more sense to simply always include AVX1 and AVX2 test prefixes, even if it so happens that for *this* particular test, codegen is identical and only the AVX prefix ends up being used. This means that whenever codegen changes in a minor way (e.g. due to a target-independent SimplyDemandedBits change that has no direct relation to X86) and a difference is introduced, you need to now figure out which new prefixes you have to add. Or drop prefixes if a codegen difference goes away. Having to manually adjust check prefixes takes away from the usual experience of "Just run update_(llc_)test_checks".

At least I personally have found the gradual migration towards disallowing unused prefixes to be more annoying than useful. I guess ergonomics could be improved if update_test_checks automatically dropped unused prefixes, but there's really no way to automatically add prefixes, without domain-specific knowledge.

Regards,

Nikita

[David Blaikie via llvm-dev]

On Wed, Feb 3, 2021 at 11:46 AM Nikita Popov <nikit...@gmail.com> wrote:
>
> On Wed, Feb 3, 2021 at 12:50 AM Fāng-ruì Sòng <mas...@google.com> wrote:
>>
>> Last October Mircea sent an RFC about (dis)allowing unused check prefixes in FileCheck: https://lists.llvm.org/pipermail/llvm-dev/2020-October/146162.html
>> (In short, if FileCheck --check-prefix or --check-prefixes specifies a prefix which does not occur in the test, FileCheck will error. Note: a `-NOT` pattern is also counted as an occurrence.)
>>
>> Mircea created a worksheet https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFbTDzC_S09e973_cwYuw/edit#gid=0 which may be a bit stale now but llvm-project has reached a state where all issues have been identified and fixed, or worked around (by opting in FileCheck --allow-unused-prefixes in some test directories, such as: clang/test/OpenMP, llvm/test/Transforms/Attributor and llvm/test/FileCheck).
>>
>> We can make a switch if the community thinks that not allowing unused prefixes is the better default: https://reviews.llvm.org/D95849
>> For downstream projects using FileCheck and lit, it should be easy to restore the previous permissive behavior
>>
>> from lit.llvm.subst import ToolSubst
>>
>> fc = ToolSubst('FileCheck', unresolved='fatal')
>> config.substitutions.insert(0, (fc.regex, 'FileCheck --allow-unused-prefixes'))
>>
>> # Note: if multiple --allow-unused-prefixes options are specified, the last wins.
>>
>>
>> Personally I am strongly in favor of disallowing unused check prefixes by default. We have identified numerous issues:
>>
>> (1) typo. A misspelled check prefix does not test what it intends to test.
>> (2) bitrot tests due to refactoring
>> (3) unspecified `-NOT` patterns. Sometimes a test uses something like `--check-prefixes=COMMON,TRUE` and `--check-prefixes=COMMON,FALSE` to test both behaviors but they forget to include a `FALSE-NO:` pattern to test that some string does not appear.
>>
>> (1) and (2) are especially common. There are indeed tests where --allow-unused-prefixes is more suitable - but they are sufficiently few that I think the default should be --allow-unused-prefixes=false.
>>
>> So, what do folks think?
>
>
> Just my 2c, but I think we should allow unused prefixes. This does catch the occasional typo, but also has a cost: Historically, certain kinds of tests simply used a certain boilerplate of check lines, because differences are common, even if they don't occur for each test. For X86 vector tests, it makes more sense to simply always include AVX1 and AVX2 test prefixes, even if it so happens that for *this* particular test, codegen is identical and only the AVX prefix ends up being used.

Is it particularly burdensome for those tests in particular to opt-in
to allowing unused prefixes on the RUN/FileCheck line? (if that's the
right tradeoff/makes things smoother compared to finding/adding the
prefixes only when they become necessary)

> This means that whenever codegen changes in a minor way (e.g. due to a target-independent SimplyDemandedBits change that has no direct relation to X86) and a difference is introduced, you need to now figure out which new prefixes you have to add. Or drop prefixes if a codegen difference goes away. Having to manually adjust check prefixes takes away from the usual experience of "Just run update_(llc_)test_checks".
>
> At least I personally have found the gradual migration towards disallowing unused prefixes to be more annoying than useful. I guess ergonomics could be improved if update_test_checks automatically dropped unused prefixes, but there's really no way to automatically add prefixes, without domain-specific knowledge.
>
> Regards,
> Nikita

[Mircea Trofin via llvm-dev]

On Wed, Feb 3, 2021 at 11:49 AM David Blaikie <dbla...@gmail.com> wrote:

On Wed, Feb 3, 2021 at 11:46 AM Nikita Popov <nikit...@gmail.com> wrote:
>
> On Wed, Feb 3, 2021 at 12:50 AM Fāng-ruì Sòng <mas...@google.com> wrote:
>>
>> Last October Mircea sent an RFC about (dis)allowing unused check prefixes in FileCheck: https://lists.llvm.org/pipermail/llvm-dev/2020-October/146162.html
>> (In short, if FileCheck --check-prefix or --check-prefixes specifies a prefix which does not occur in the test, FileCheck will error. Note: a `-NOT` pattern is also counted as an occurrence.)
>>
>> Mircea created a worksheet https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFbTDzC_S09e973_cwYuw/edit#gid=0 which may be a bit stale now but llvm-project has reached a state where all issues have been identified and fixed, or worked around (by opting in FileCheck --allow-unused-prefixes in some test directories, such as: clang/test/OpenMP, llvm/test/Transforms/Attributor and llvm/test/FileCheck).
>>
>> We can make a switch if the community thinks that not allowing unused prefixes is the better default: https://reviews.llvm.org/D95849
>> For downstream projects using FileCheck and lit, it should be easy to restore the previous permissive behavior
>>
>> from lit.llvm.subst import ToolSubst
>>
>> fc = ToolSubst('FileCheck', unresolved='fatal')
>> config.substitutions.insert(0, (fc.regex, 'FileCheck --allow-unused-prefixes'))
>>
>> # Note: if multiple --allow-unused-prefixes options are specified, the last wins.
>>
>>
>> Personally I am strongly in favor of disallowing unused check prefixes by default. We have identified numerous issues:
>>
>> (1) typo. A misspelled check prefix does not test what it intends to test.
>> (2) bitrot tests due to refactoring
>> (3) unspecified `-NOT` patterns. Sometimes a test uses something like `--check-prefixes=COMMON,TRUE` and `--check-prefixes=COMMON,FALSE` to test both behaviors but they forget to include a `FALSE-NO:` pattern to test that some string does not appear.
>>
>> (1) and (2) are especially common. There are indeed tests where --allow-unused-prefixes is more suitable - but they are sufficiently few that I think the default should be --allow-unused-prefixes=false.
>>
>> So, what do folks think?
>
>
> Just my 2c, but I think we should allow unused prefixes. This does catch the occasional typo, but also has a cost: Historically, certain kinds of tests simply used a certain boilerplate of check lines, because differences are common, even if they don't occur for each test. For X86 vector tests, it makes more sense to simply always include AVX1 and AVX2 test prefixes, even if it so happens that for *this* particular test, codegen is identical and only the AVX prefix ends up being used.

Is it particularly burdensome for those tests in particular to opt-in
to allowing unused prefixes on the RUN/FileCheck line? (if that's the
right tradeoff/makes things smoother compared to finding/adding the
prefixes only when they become necessary)

Another option available is to whole-sale opt in those directories with tests wishing to make this tradeoff, via lit.local.cfg, like we already did for the Attributor (and @maskray summarized above)

[James Y Knight via llvm-dev]

On Wed, Feb 3, 2021 at 2:46 PM Nikita Popov via llvm-dev <llvm...@lists.llvm.org> wrote:

Just my 2c, but I think we should allow unused prefixes. This does catch the occasional typo, but also has a cost: Historically, certain kinds of tests simply used a certain boilerplate of check lines, because differences are common, even if they don't occur for each test. For X86 vector tests, it makes more sense to simply always include AVX1 and AVX2 test prefixes, even if it so happens that for *this* particular test, codegen is identical and only the AVX prefix ends up being used. This means that whenever codegen changes in a minor way (e.g. due to a target-independent SimplyDemandedBits change that has no direct relation to X86) and a difference is introduced, you need to now figure out which new prefixes you have to add. Or drop prefixes if a codegen difference goes away. Having to manually adjust check prefixes takes away from the usual experience of "Just run update_(llc_)test_checks".

At least I personally have found the gradual migration towards disallowing unused prefixes to be more annoying than useful. I guess ergonomics could be improved if update_test_checks automatically dropped unused prefixes, but there's really no way to automatically add prefixes, without domain-specific knowledge.

ISTM That the tests using update_test_checks should probably opt into allowing unused prefixes as a matter of course.

With a set of auto-updated checks, an unused prefix is unlikely to be due to a mistake, like it would indicate be for a manually maintained test. Maybe update_test_checks could even add the right argument to FileCheck automatically.

[Fāng-ruì Sòng via llvm-dev]

The comment may be related to fixes such as

7979f24954ed6c1bfac8a9961fee69a44822f2b6 "[test] Fix some unused check prefixes in test/Analysis/CostModel/X86"
and
f4467c4d3b6c65d2a0d799badb1edf233e829162 "[CodeGen][X86] Remove some unused check-prefixes and regenerate tests."

In the updates, a common pattern is to replace multi-level

--check-prefixes=CHECK,SSE,SSE1
--check-prefixes=CHECK,SSE,SSE2

with

--check-prefixes=CHECK,SSE1
--check-prefixes=CHECK,SSE2

because SSE ends up to be unneeded.

I can see arguments that 'SSE' may be needed - if the code generation
changes and SSE1/SSE2 happen to have new common, keeping 'SSE' can allow
shared check lines with 'SSE:'.

However, I personally don't consider it as good use of sharing.
Tree-style sharing is fairly brittle and difficult for automatic tools
like update{,_llc,_analyzer}_test_checks.py to update.
Many times two-level sharing is sufficient and sometimes just
duplicating everything is more robust - in the cost of verbose check
prefixes.

>At least I personally have found the gradual migration towards disallowing
>unused prefixes to be more annoying than useful. I guess ergonomics could
>be improved if update_test_checks automatically dropped unused prefixes,
>but there's really no way to automatically add prefixes, without
>domain-specific knowledge.
>
>Regards,
>Nikita

[Nikita Popov via llvm-dev]

That's a great observation. If update_test_checks is used, then typos aren't really possible and unused check prefixes are only unnecessary at worst (and usually "currently unnecessary"). Making a distinction based on that seems like a nice solution to me.

Regards,

Nikita

[Fāng-ruì Sòng via llvm-dev]

Thanks for the feedback! llvm/test/CodeGen has already defaulted to
--allow-unused-prefixes=false.
From this discussion, update{,_llc,_analyzer}_test_checks can be
improved to add --allow-unused-prefixes on demand.
(Such tests do exist but are not too many.)
So I will assume that the blocker has been resolved.

--
宋方睿

[Fāng-ruì Sòng via llvm-dev]

I'll submit https://reviews.llvm.org/D95849 next week.

(We actually have another thread discussing the
update_*_test_checks.py's problem
(https://lists.llvm.org/pipermail/llvm-dev/2021-February/148367.html).
Hope we can find folks who are willing to improve the situation. (Not
affected by this FileCheck default change as far as I can see :)))