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

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 <mtrofin at 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 <pengfei.wang at
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 <maskray at google.com>
>> Sent: Friday, November 6, 2020 3:50 AM
>> To: Matt Arsenault <Matthew.Arsenault at amd.com>; Alexey.Bataev
at ibm.com;
>> Tim Northover <t.p.northover at gmail.com>; Wang, Pengfei <
>> pengfei.wang at intel.com>
>> Cc: Mircea Trofin <mtrofin at google.com>; David Blaikie
<dblaikie at gmail.com>;
>> LLVM Dev <llvm-dev at 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-dev at lists.llvm.org> wrote:
>> >
>> >
>> >
>> > On Thu, Nov 5, 2020 at 10:46 AM Mircea Trofin <mtrofin at
google.com>
>> wrote:
>> >>
>> >>
>> >>
>> >> On Thu, Nov 5, 2020 at 10:40 AM David Blaikie <dblaikie at
gmail.com>
>> wrote:
>> >>>
>> >>>
>> >>>
>> >>> On Thu, Nov 5, 2020 at 7:30 AM Mircea Trofin via llvm-dev
<
>> llvm-dev at 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
<https://teams.googleplex.com/u/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
<mtrofin at 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
<mtrofin at 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-dev at lists.llvm.org
>> >>>>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> >
>> > _______________________________________________
>> > LLVM Developers mailing list
>> > llvm-dev at lists.llvm.org
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
>>
>> --
>> 宋方睿
>>
>
-- 
宋方睿
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210202/85b5f042/attachment.html>

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.

On Tue, 2 Feb 2021 at 23:50, 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?
>
> On Mon, Nov 16, 2020 at 8:22 AM Mircea Trofin <mtrofin at 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 <pengfei.wang at
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 <maskray at google.com>
>>> Sent: Friday, November 6, 2020 3:50 AM
>>> To: Matt Arsenault <Matthew.Arsenault at amd.com>;
Alexey.Bataev at ibm.com;
>>> Tim Northover <t.p.northover at gmail.com>; Wang, Pengfei
<
>>> pengfei.wang at intel.com>
>>> Cc: Mircea Trofin <mtrofin at google.com>; David Blaikie <
>>> dblaikie at gmail.com>; LLVM Dev <llvm-dev at
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-dev at lists.llvm.org> wrote:
>>> >
>>> >
>>> >
>>> > On Thu, Nov 5, 2020 at 10:46 AM Mircea Trofin <mtrofin at
google.com>
>>> wrote:
>>> >>
>>> >>
>>> >>
>>> >> On Thu, Nov 5, 2020 at 10:40 AM David Blaikie <dblaikie
at gmail.com>
>>> wrote:
>>> >>>
>>> >>>
>>> >>>
>>> >>> On Thu, Nov 5, 2020 at 7:30 AM Mircea Trofin via
llvm-dev <
>>> llvm-dev at 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
<https://teams.googleplex.com/u/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
<mtrofin at 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
<mtrofin at 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-dev at lists.llvm.org
>>> >>>>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>> >
>>> > _______________________________________________
>>> > LLVM Developers mailing list
>>> > llvm-dev at lists.llvm.org
>>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>>
>>>
>>> --
>>> 宋方睿
>>>
>>
>
> --
> 宋方睿
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210203/ede0fd60/attachment.html>

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>