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

[Mircea Trofin via llvm-dev]

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

> 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)
>

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)


> > 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/a712b66b/attachment.html>