[PATCH] D47106: [FileCheck] Make CHECK-DAG non-overlapping

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added inline comments.


================
Comment at: test/FileCheck/check-dag-overlap.txt:12
+
+; IdentPat:     {{^}}__IdentPat
+; IdentPat-DAG: {{^}}add [[REG1:r[0-9]+]], {{r[0-9]+}}, {{r[0-9]+}}
----------------
probinson wrote:
> jdenny wrote:
> > probinson wrote:
> > > Ordinarily these delimiters would be using -LABEL; is there some reason not to?
> > I was mimicking check-dag-xfails.txt.  I was thinking -LABEL wouldn't work well because each RUN would see only one -LABEL, but I suppose all RUNs could share a common CHECK prefix for -LABEL.  I'll work on that.  Thanks.
> Oh, makes sense, yes.  Never mind the -LABEL stuff if you haven't already done it.  The test will be easier to read if you don't mix CHECK in with the run-specific prefix names.
No, I haven't started. I'll skip this one then. 


================
Comment at: test/FileCheck/check-dag-overlap.txt:467
+; DagNotDag:     {{^}}__DagNotDag
\ No newline at end of file

----------------
probinson wrote:
> jdenny wrote:
> > probinson wrote:
> > > Reading this in the overlapping mode:  The X set of DAGs will all match on line 424, the Y set will all match on line 425, therefore the NOTs for rule 1 will correctly not match anything.  The Z set of DAGs will all match on line 428, therefore the NOTs for rule 2 will correctly not match anything.  Then the NOTs for rule 3 will match, making the run fail.
> > > 
> > > Is that what you intended?  Naively I would have thought you'd try to break each of the rules, but that's not what the test does.
> > > Reading this in the overlapping mode: The X set of DAGs will all match on line 424,
> > 
> > Yes.
> > 
> > > the Y set will all match on line 425
> > 
> > No.  y also matches on line 424 because overlapping is permitted, so we get a reordering complaint.  You can confirm this with the -vv option.
> > 
> > > Is that what you intended? Naively I would have thought you'd try to break each of the rules, but that's not what the test does.
> > 
> > It breaks the first rule because of -allow-deprecated-dag-overlap.
> > 
> > With non-overlapping matches, it breaks the second rule if I remove the following line from CheckString::CheckDag:
> > 
> >       Matches.push_back(Match{MatchPos, MatchPos + MatchLen})
> > 
> > (I had that bug at one point.)
> > 
> > While rule 3 is exercised, I don't know how to break it without breaking rule 1 or 2 because it's a consequence of rules 1 and 2.
> >> the Y set will all match on line 425
> >
> > No. y also matches on line 424 because overlapping is permitted, so we get a reordering complaint. You can confirm this with the -vv option.
> 
> Huh.  I would have expected the -NOT directives for rule 1 to break up the DAG lines into separate X and Y groups, and the Y group's starting position would necessarily have been the end of the X group's last match.
> At least, that appears to be the intended behavior of the `IdentPatNot` part of the test?
> I admit I have not downloaded and applied the patches, I'll try to do that tomorrow.
The reordering check is a bit odd, regardless of my patch.  It looks for y from the start of X, but it looks for the rest of Y from the end of X. My patch makes sure y doesn't overlap with members of X.


https://reviews.llvm.org/D47106