[PATCH] D56789: Allow CHECK-SAME, NEXT and EMPTY after CHECK-DAG

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added a comment.

In D56789#1368287 <https://reviews.llvm.org/D56789#1368287>, @jdenny wrote:

> Hi Thomas,
>
> Thanks for working to clean up this area.
>
> As I understand it, `CHECK-NEXT`, `CHECK-SAME`, and `CHECK-EMPTY` currently can appear after non-initial `CHECK-DAG` blocks, and this patch intends to permit them after initial `CHECK-DAG` blocks as well.  I agree with Paul that the title should be adjusted to reflect that.


Correct.

> However, I remember some disagreement in the past as to whether these directives should be permitted after any `CHECK-DAG` at all.  We should probably resolve any remaining disagreement before relaxing things further.

Indeed, I was not aware of the disagreement.

> Consider the example that this patch adds to the docs:
> 
>   // CHECK-DAG: mov r0, #0
>   // CHECK-DAG: mov r1, #1
>   // CHECK-NEXT: add r2, r0, r1
> 
> 
> Unmatched lines are permitted to appear between `CHECK-DAG` matches, but unmatched lines are not permitted to appear between whichever `CHECK-DAG` match happens to occur last and the `CHECK-NEXT` match.  Is that a realistic use case?

Fair enough, any clobber can happen between the two, forgot about that. Note that I can see several existing tests in the testsuite that have CHECK-NEXT right after CHECK-DAG which are probably not as strict in terms of testing as their author were expecting, e.g.

test/Analysis/LazyValueAnalysis/lvi-after-jumpthreading.ll
test/Bitcode/thinlto-function-summary-originalnames.ll
test/CodeGen/AArch64/arm64-ccmp.ll
test/CodeGen/AArch64/arm64-indexed-vector-ldst.ll (though it's a single DAG between a CHECK and CHECK-NEXT so it's a special case)
test/CodeGen/AArch64/f16-instructions.ll

and many more.

> A realistic use case would probably involve something like a `CHECK-DAG-NEXT` directive that doesn't permit intervening lines anywhere in the block.  Or should the above example actually indicate exactly that?
> 
> Do you have a use case from a real test to inform this discussion?

Agreed, something like CHECK-DAG-NEXT would be better suited. The requirement of CHECK-DAG with CHECK-NEXT was mentioned to me by a coworker and I noticed the weird inconsistency regarding whether CHECK-DAG is the first check directives or not and thought I'd fix it. The behaviour we need is indeed reordering without any line not checked for in between using CHECK-DAG is not the right solution. So I guess the correct approach would be to error out on CHECK-DAG with CHECK-NEXT, CHECK-SAME or CHECK-EMPTY, probably with some flag to enable the old behavior while tests are updated.

Regarding CHECK-DAG-NEXT, I wonder how it would interact with CHECK-DAG? Perhaps we should introduce some sort of only-tested region (e.g. CHECK-ONLY-START and CHECK-ONLY-END with better name) where CHECK-DAG inside would still be able to be reordered but only what is tested by a CHECK-DAG is allowed. It could also allow CHECK directive inside which would behave as CHECK-NEXT. How does that sound?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56789/new/

https://reviews.llvm.org/D56789