[llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes

Mon Nov 16 08:22:33 PST 2020

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/20201116/53733e3c/attachment.html>