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

On Wed, Feb 3, 2021 at 9:02 PM James Y Knight <jyknight at google.com>
wrote:
>
>
> 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.
>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210203/a4c0bd90/attachment.html>

On Wed, Feb 3, 2021 at 12:12 PM Nikita Popov <nikita.ppv at gmail.com>
wrote:
>
> On Wed, Feb 3, 2021 at 9:02 PM James Y Knight <jyknight at
google.com> wrote:
>>
>>
>>
>> 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.
>
>
> 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
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.



-- 
宋方睿