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

On Wed, Feb 3, 2021 at 12:50 AM Fāng-ruì Sòng <maskray at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210203/cb403b6f/attachment.html>

On Wed, Feb 3, 2021 at 11:46 AM Nikita Popov <nikita.ppv at gmail.com>
wrote:
>
> On Wed, Feb 3, 2021 at 12:50 AM Fāng-ruì Sòng <maskray at 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

On Wed, Feb 3, 2021 at 2:46 PM Nikita Popov via llvm-dev <
llvm-dev at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210203/7ae087a5/attachment.html>

On 2021-02-03, Nikita Popov wrote:
>On Wed, Feb 3, 2021 at 12:50 AM Fāng-ruì Sòng <maskray at 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".
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