Page MenuHomePhabricator

[FileCheck] Fix search ranges for DAG-NOT-DAG
ClosedPublic

Authored by jdenny on Jul 5 2018, 12:02 PM.

Details

Summary

A DAG-NOT-DAG is a CHECK-DAG group, X, followed by a CHECK-NOT group,
N, followed by a CHECK-DAG group, Y. Let y be the initial directive
of Y. This patch makes the following changes to the behavior:

  1. Directives in N can no longer match within part of Y's match range just because y happens not to be the earliest match from Y. Specifically, this patch withdraws N's search range end from y's match range start to Y's match range start.
  2. y can no longer match within X's match range, where a y match produced a reordering complaint, which is thus no longer possible. Specifically, this patch withdraws y's search range start from X's permitted range start to X's match range end, which was already the search range start for other members of Y.

Both of these changes can only increase the number of test passes: #1
constrains the ability of CHECK-NOTs to match, and #2 expands the
ability of CHECK-DAGs to match without complaints.

These changes are based on discussions at:

<http://lists.llvm.org/pipermail/llvm-dev/2018-May/123550.html>
<https://reviews.llvm.org/D47106>

which conclude that:

  1. These changes simplify the FileCheck conceptual model. First, it makes search ranges for DAG-NOT-DAG more consistent with other cases. Second, it was confusing that y was treated differently from the rest of Y.
  2. These changes add theoretical use cases for DAG-NOT-DAG that had no obvious means to be expressed otherwise. We can justify the first half of this assertion with the observation that these changes can only increase the number of test passes.
  3. Reordering detection for DAG-NOT-DAG had no obvious real benefit.

We don't have evidence from real uses cases to help us debate
conclusions #2 and #3, but #1 at least seems intuitive.

Diff Detail

Repository
rL LLVM

Event Timeline

jdenny updated this revision to Diff 156525.Jul 20 2018, 10:06 AM
jdenny edited the summary of this revision. (Show Details)

Ping. Rebased.

probinson accepted this revision.Jul 20 2018, 10:44 AM

Sorry, lost track of this one. One comment suggestion and LGTM.

llvm/utils/FileCheck/FileCheck.cpp
1299 ↗(On Diff #156525)

Range-based for loops are now so canonical, you should have a comment explaining why this isn't. Especially because the reason is pretty far away in the source.

This revision is now accepted and ready to land.Jul 20 2018, 10:44 AM

Sorry, lost track of this one. One comment suggestion and LGTM.

Thanks! I'll make that change and commit soon.

This revision was automatically updated to reflect the committed changes.