This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU, test] Fix use of undef FileCheck var
ClosedPublic

Authored by thopre on Apr 4 2021, 1:47 PM.

Details

Summary

Test CodeGen/AMDGPU/amdgpu.private-memory.ll and
CodeGen/AMDGPU/private-memory-r600.ll have a block of CHECK directives
whose prefix is inconsistent: R600-CHECK Vs R600. This leads to a
R600-NOT directive using an undefined CHAN variable due to R600-CHECK
directives never being considered by FileCheck. Fixing the prefix leads
to the testcase failing. As per https://reviews.llvm.org/D99865#2675235
this commit removes the directives instead since it is not possible to
write a reliable check.

Diff Detail

Event Timeline

thopre created this revision.Apr 4 2021, 1:47 PM
thopre requested review of this revision.Apr 4 2021, 1:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2021, 1:47 PM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm accepted this revision.Apr 7 2021, 2:30 PM
arsenm added a subscriber: arsenm.

It's really hard to write checks for R600, but it's in maintenance mode. The comment doesn't really make sense to me. I doubt this check was ever really correct or stable. You can't really write this test stably and expect specific things in T registers at a specific time. I guess just go with this is it passes?

This revision is now accepted and ready to land.Apr 7 2021, 2:30 PM
thopre added a comment.Apr 7 2021, 2:51 PM

It's really hard to write checks for R600, but it's in maintenance mode. The comment doesn't really make sense to me. I doubt this check was ever really correct or stable. You can't really write this test stably and expect specific things in T registers at a specific time. I guess just go with this is it passes?

Unfortunately the test case as updated by this diff FAILs, as can be seen above. Unfortunately the current testcase only succeeds due to a bug in FileCheck which I have a patch for. FileCheck currently count an undefined variable as a match failure which makes a CHECK-NOT with an undefined variable succeed. Does the comment make sense with the old CHECKS (see https://reviews.llvm.org/rG880a80ad07b6ea7ead3a842fc03c74c2247c9486)?

Should we remove those checks altogether?

arsenm added a comment.Apr 7 2021, 3:02 PM

It's really hard to write checks for R600, but it's in maintenance mode. The comment doesn't really make sense to me. I doubt this check was ever really correct or stable. You can't really write this test stably and expect specific things in T registers at a specific time. I guess just go with this is it passes?

Unfortunately the test case as updated by this diff FAILs, as can be seen above. Unfortunately the current testcase only succeeds due to a bug in FileCheck which I have a patch for. FileCheck currently count an undefined variable as a match failure which makes a CHECK-NOT with an undefined variable succeed. Does the comment make sense with the old CHECKS (see https://reviews.llvm.org/rG880a80ad07b6ea7ead3a842fc03c74c2247c9486)?

Should we remove those checks altogether?

Might as well delete them then. I don't know how you can really write a reasonable check for what the comment describes

thopre updated this revision to Diff 335937.Apr 7 2021, 3:28 PM

Remove directives instead

thopre retitled this revision from [RFC, AMDGPU, test] Fix use of undef FileCheck var to [AMDGPU, test] Fix use of undef FileCheck var.Apr 7 2021, 3:28 PM
thopre edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.