[PATCH] D110143: [Utils] Replace llc with cat for tests

[Alexander Richardson via Phabricator via llvm-commits]

arichardson accepted this revision.
arichardson added a comment.
This revision is now accepted and ready to land.

Thanks for moving it to common. This generally LGTM just a suggestion to avoid %T.

All the other update scripts seem to use `invoke_tool()` so we should be able to drop %s handling from those scripts too? I guess substitution twice doesn't do any harm but it would be good to remove duplication to make future refactoring easier.



================
Comment at: llvm/test/tools/UpdateTestChecks/update_llc_test_checks/amdgpu-no-merge-comments.test:5
+# RUN: cp -f %S/Inputs/amdgpu_no_merge_comments-O0.s %T/ && cp -f %S/Inputs/amdgpu_no_merge_comments-O3.s %T/
+# RUN: cp -f %S/Inputs/amdgpu_no_merge_comments.ll %t.ll && %update_llc_test_checks --llc-binary cat %t.ll
 # RUN: diff -u %S/Inputs/amdgpu_no_merge_comments.ll.expected %t.ll
----------------
sebastian-ne wrote:
> arichardson wrote:
> > IMO this should be in a separate test file since it's not checking "that functions with different IR comments in the output are not merged" but instead is checking the %S lit substitution
> Thanks for your review!
> 
> This is still testing "that functions with different IR comments in the output are not merged" with the `%update_llc_test_checks --llc-binary cat %t.ll` command.
> The %S substitution in this file was working before (it is already used before this patch).
> The two copies of the .s files are needed because %update_llc_test_checks is run on a temporary file in `build/test/tools/UpdateTestChecks/update_llc_test_checks/Output/` and the tested .ll file `amdgpu_no_merge_comments.ll` expects the two .s files to be in the same directory.
> 
> The added `%S` support is only for running %update_llc_test_checks. The two RUN lines in `amdgpu_no_merge_comments.ll` are exercising this code (although not exactly for testing, but through necessity to somehow reference the .s files).
I would try to avoid the %T substitituion here, can you use `%t.ll-O3.s` and `%t.ll-0O.s`? Then it should be possible to use %s-O3.s in the tests instead of %S?

I think it might also make sense to add a comment above this RUN line. Maybe something like
`We override the llc binary with cat to make this test independent of future amdgpu assembly output changes`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110143