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

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

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