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

[Paul Robinson via Phabricator via llvm-commits]

probinson 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]+}}
----------------
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.


================
Comment at: test/FileCheck/check-dag-overlap.txt:34
+; IdentPatNot-NOT: {{^}}xor
+; IdentPatNot-DAG: {{^}}mul r5, r10, r11
+; IdentPatNot:     {{^}}__IdentPatNot
----------------
jdenny wrote:
> probinson wrote:
> > Normally I'd say a single DAG after a NOT is more misleading than helpful; unless you changed some behavior here?
> I was just trying to prove that this case is handled sanely when overlapping matches must be skipped.  I don't mean to imply anything about whether it's a good way to write test cases.
OK.


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

----------------
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.


https://reviews.llvm.org/D47106