llvm dev - May 2018 - RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Paul Robinson <paul.robinson at sony.com> writes:
>> From: Joel E. Denny [mailto:jdenny.ornl at gmail.com]
>>> On Mon, May 7, 2018 at 12:56 PM, <paul.robinson at sony.com>
wrote:
>>>> 1. In a CHECK-DAG group, don't let the matches for patterns
overlap.
>>>> 2. Add a new CHECK-DAG-N directive, where N is some integer, to
express
>>>> that a pattern must have N non-overlapping matches.
>>>
>>> I think #1 is much more intuitive and easy to describe/document
than #2.
>>> Changing the meaning of DAG in that way is highly unlikely to
affect any
>>> existing test, IMO.  And if it does, my first expectation is that
the test
>>> wasn't doing what the author intended anyway.
>>
>> I implemented #1, and I now have 105 new unexpected tests failures from
>> llvm core (58), clang (45), openmp runtime (1), and compiler-rt (1)
test
>> suites.
...
>> Here are some potential paths forward:
>>
>> P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go back to
>> #2 (CHECK-DAG-N).  I don't prefer this, but it would be less work.
>>
>> P2. Make CHECK-DAG non-overlapping but fix all test cases before
>> committing that change.  Patch reviews will surely be slow.
>>
>> P3. Make non-overlapping CHECK-DAG a special mode that's off by
default
>> until all test suites have been fixed.
>
> I vote for P3.  It's easier to divide up the work if the feature is in
> upstream FileCheck, and it's easy for someone doing the work to enable
> the feature in their local branch.  I would not bother making it a
> command-line switch, unless we really do come up with cases where the
> overlapping matches are absolutely the right solution for some class of
> tests.
I feel like a variant of this where this mode is on by default would
make more sense. That is,

1. Modify CHECK-DAG so that overlapping patterns aren't allow, unless
   --deprecated-allow-overlapping-check-dag or something is passed to
   FileCheck.
2. Add the flag to the 105 current test failures
3. File bugs and/or create reviews to remove the flag from each of the
   tests.

There are two reasons I prefer this approach. First, new tests are
"correct by default" while we clean up the existing problematic tests.
Second, out of tree work has an easy path forward and reasonable
forewarning. They can add the flag temporarily and fix problematic tests
when time permits, rather it suddenly breaking their CI whenever open
source flips the switch.
> I suggest you file a bug with the complete list of test failures that
> non-overlapping CHECK-DAG finds.  That will help keep track of the work,
> and if anyone else feels moved to pick up some part of it, they can make
> notes in the bug so we can try to avoid duplicating effort.  Hopefully
> I can devote some time to this myself, I think it's how CHECK-DAG
should
> have been done in the first place and I'm sad we didn't catch it at
the
> time.
>
>> By the way, I think it would be generally useful for FileCheck to have
>> a debugging mode that prints every match.  It would be particularly
>> useful while migrating to a non-overlapping CHECK-DAG.
>>
>> Thoughts?
>>
>> Joel
>
> A debugging mode like that sounds like a useful feature for writing a
> test, independent of the migration aid.  Do it.
+1

> -----Original Message-----
> From: Justin Bogner [mailto:justin at justinbogner.com] On Behalf Of Justin
> Bogner
> Sent: Wednesday, May 16, 2018 12:16 PM
> To: llvm-dev
> Cc: jdenny.ornl at gmail.com; Robinson, Paul
> Subject: Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple
> occurrences of string
> 
> Paul Robinson <paul.robinson at sony.com> writes:
> >> From: Joel E. Denny [mailto:jdenny.ornl at gmail.com]
> >>> On Mon, May 7, 2018 at 12:56 PM, <paul.robinson at
sony.com> wrote:
> >>>> 1. In a CHECK-DAG group, don't let the matches for
patterns overlap.
> >>>> 2. Add a new CHECK-DAG-N directive, where N is some
integer, to
> express
> >>>> that a pattern must have N non-overlapping matches.
> >>>
> >>> I think #1 is much more intuitive and easy to
describe/document than
> #2.
> >>> Changing the meaning of DAG in that way is highly unlikely to
affect
> any
> >>> existing test, IMO.  And if it does, my first expectation is
that the
> test
> >>> wasn't doing what the author intended anyway.
> >>
> >> I implemented #1, and I now have 105 new unexpected tests failures
from
> >> llvm core (58), clang (45), openmp runtime (1), and compiler-rt
(1)
> test
> >> suites.
> ...
> >> Here are some potential paths forward:
> >>
> >> P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go back
to
> >> #2 (CHECK-DAG-N).  I don't prefer this, but it would be less
work.
> >>
> >> P2. Make CHECK-DAG non-overlapping but fix all test cases before
> >> committing that change.  Patch reviews will surely be slow.
> >>
> >> P3. Make non-overlapping CHECK-DAG a special mode that's off
by default
> >> until all test suites have been fixed.
> >
> > I vote for P3.  It's easier to divide up the work if the feature
is in
> > upstream FileCheck, and it's easy for someone doing the work to
enable
> > the feature in their local branch.  I would not bother making it a
> > command-line switch, unless we really do come up with cases where the
> > overlapping matches are absolutely the right solution for some class
of
> > tests.
> 
> I feel like a variant of this where this mode is on by default would
> make more sense. That is,
> 
> 1. Modify CHECK-DAG so that overlapping patterns aren't allow, unless
>    --deprecated-allow-overlapping-check-dag or something is passed to
>    FileCheck.
> 2. Add the flag to the 105 current test failures
> 3. File bugs and/or create reviews to remove the flag from each of the
>    tests.
> 
> There are two reasons I prefer this approach. First, new tests are
> "correct by default" while we clean up the existing problematic
tests.
> Second, out of tree work has an easy path forward and reasonable
> forewarning. They can add the flag temporarily and fix problematic tests
> when time permits, rather it suddenly breaking their CI whenever open
> source flips the switch.
Those are excellent points, and that approach totally makes sense.
--paulr

On Wed, May 16, 2018 at 12:24 PM, <paul.robinson at sony.com> wrote:
>
>
> > -----Original Message-----
> > From: Justin Bogner [mailto:justin at justinbogner.com] On Behalf Of
Justin
> > Bogner
> > Sent: Wednesday, May 16, 2018 12:16 PM
> > To: llvm-dev
> > Cc: jdenny.ornl at gmail.com; Robinson, Paul
> > Subject: Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple
> > occurrences of string
> >
> > Paul Robinson <paul.robinson at sony.com> writes:
> > >> From: Joel E. Denny [mailto:jdenny.ornl at gmail.com]
> > >>> On Mon, May 7, 2018 at 12:56 PM, <paul.robinson at
sony.com> wrote:
> > >>>> 1. In a CHECK-DAG group, don't let the matches
for patterns overlap.
> > >>>> 2. Add a new CHECK-DAG-N directive, where N is some
integer, to
> > express
> > >>>> that a pattern must have N non-overlapping matches.
> > >>>
> > >>> I think #1 is much more intuitive and easy to
describe/document than
> > #2.
> > >>> Changing the meaning of DAG in that way is highly
unlikely to affect
> > any
> > >>> existing test, IMO.  And if it does, my first expectation
is that the
> > test
> > >>> wasn't doing what the author intended anyway.
> > >>
> > >> I implemented #1, and I now have 105 new unexpected tests
failures
> from
> > >> llvm core (58), clang (45), openmp runtime (1), and
compiler-rt (1)
> > test
> > >> suites.
> > ...
> > >> Here are some potential paths forward:
> > >>
> > >> P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go
back to
> > >> #2 (CHECK-DAG-N).  I don't prefer this, but it would be
less work.
> > >>
> > >> P2. Make CHECK-DAG non-overlapping but fix all test cases
before
> > >> committing that change.  Patch reviews will surely be slow.
> > >>
> > >> P3. Make non-overlapping CHECK-DAG a special mode that's
off by
> default
> > >> until all test suites have been fixed.
> > >
> > > I vote for P3.  It's easier to divide up the work if the
feature is in
> > > upstream FileCheck, and it's easy for someone doing the work
to enable
> > > the feature in their local branch.  I would not bother making it
a
> > > command-line switch, unless we really do come up with cases where
the
> > > overlapping matches are absolutely the right solution for some
class of
> > > tests.
> >
> > I feel like a variant of this where this mode is on by default would
> > make more sense. That is,
> >
> > 1. Modify CHECK-DAG so that overlapping patterns aren't allow,
unless
> >    --deprecated-allow-overlapping-check-dag or something is passed to
> >    FileCheck.
> > 2. Add the flag to the 105 current test failures
> > 3. File bugs and/or create reviews to remove the flag from each of the
> >    tests.
> >
> > There are two reasons I prefer this approach. First, new tests are
> > "correct by default" while we clean up the existing
problematic tests.
> > Second, out of tree work has an easy path forward and reasonable
> > forewarning. They can add the flag temporarily and fix problematic
tests
> > when time permits, rather it suddenly breaking their CI whenever open
> > source flips the switch.
>
> Those are excellent points, and that approach totally makes sense.
> --paulr
>
>
Make sense.  I'll work on putting the patch in phabricator and creating the
bugzilla issue.  I'll get to implementing the debugging mode eventually.

Thanks.

Joel
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180516/f6b3b84a/attachment.html>