[Utils] Replace llc with cat for tests

[sebastian-ne]

Summary

Make the update_llc_test_checks script test independant of llc behavior
by using cat with static files to simulate llc output.

This allows changing llc without breaking the script test case.

The update script is executed in a temporary directory, so the
llc-generated assembly files are copied there. %T is deprecated, but it
allows copying a file with a predictable filename.

[foad]

Looks great, thanks! Will it work on Windows?

[arichardson]

llvm/utils/update_llc_test_checks.py

94 ↗

(On Diff #373828)

Doing this in common.py would also fix the other update scripts instead of having to duplicate it everywhere.

Also I wonder if we defer to lit's expansion code instead of re-implementing lit features here?

[arichardson]

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/amdgpu-no-merge-comments.test

5

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

[sebastian-ne]

Implement substitutions in common.py.

Also I wonder if we defer to lit's expansion code instead of re-implementing lit features here?

That would be nice, but changing that is more involved.

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/amdgpu-no-merge-comments.test

5

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

[sebastian-ne]

Remove left over dead code

[foad]

Implement substitutions in common.py.

Does that mean they can be removed from the individual update_* tools? I guess that can be done as follow-ups.

[sebastian-ne]

In D110143#3012137, @foad wrote:

Implement substitutions in common.py.

Does that mean they can be removed from the individual update_* tools? I guess that can be done as follow-ups.

Most update scripts don’t apply these substitution so far. update_cc_test_checks.py doesn’t use the common functions to start the process, but it could use get- and applySubstitutions.
I’ll do a follow-up if this gets merged.

[foad]

Looks fine to me once @arichardson is happy.

[arichardson]

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.

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/amdgpu-no-merge-comments.test

5

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?

[sebastian-ne]

Good idea, I added a comment.

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.

%s is not substituted in the scripts (except update_cc_test_checks.py), it is just removed with .replace('< %s', '').replace('%s', ''). We can move that to invoke_tool() if you think it makes sense?

Then it should be possible to use %s-O3.s in the tests instead of %S?

I tried exactly that before, but for the above reason, that %s is removed and not substituted, this doesn’t work (it tries to run cat -03.s).

[arichardson]

In D110143#3012500, @sebastian-ne wrote:

Good idea, I added a comment.

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.

%s is not substituted in the scripts (except update_cc_test_checks.py), it is just removed with .replace('< %s', '').replace('%s', ''). We can move that to invoke_tool() if you think it makes sense?

Then it should be possible to use %s-O3.s in the tests instead of %S?

I tried exactly that before, but for the above reason, that %s is removed and not substituted, this doesn’t work (it tries to run cat -03.s).

Does the following work:

# RUN: cp -f %S/Inputs/amdgpu_no_merge_comments-O0.s %t-O0.s && cp -f %S/Inputs/amdgpu_no_merge_comments-O3.s %t-O3.s
# 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

And then inside the actual test

; llc is replaced with cat, we just simulate llc by printing text from these files
; RUN: llc %S/amdgpu_no_merge_comments.tmp-O0.s | FileCheck -check-prefixes=GCN,GFX9-O0 %s
; RUN: llc %S/amdgpu_no_merge_comments.tmp-O3.s | FileCheck -check-prefixes=GCN,GFX9-O3 %s

Not sure if that's much better than depending on %T though.

llvm/utils/UpdateTestChecks/common.py

174

Just noticed that we should also be using applySubstitutions here.

[sebastian-ne]

Use applySubstitutions in one more place.

Not sure if that's much better than depending on %T though.

Ideally the update scripts would share the code with the lit script and we coud just use %t.
I’ll leave it at %T for now because that doesn’t depend on implementation details.

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/amdgpu_no_merge_comments-O0.s

+  .text
+  .section  .AMDGPU.config
+  .long 47176
+  .long 0
+  .long 47180
+  .long 0
+  .long 47200
+  .long 0
+  .long 4
+  .long 0
+  .long 8
+  .long 0
+  .text
+  .hidden main                            ; -- Begin function main
+  .globl  main
+  .p2align  2
+  .type main,@function
+main:                                   ; @main
+; %bb.0:
+  s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+  v_add_u32_e64 v1, v0, v0
+                                        ; implicit-def: $sgpr4
+                                        ; implicit-def: $sgpr4
+  v_mul_lo_u32 v0, v1, v0
+  v_sub_u32_e64 v0, v0, v1
+  s_setpc_b64 s[30:31]
+.Lfunc_end0:
+  .size main, .Lfunc_end0-main
+                                        ; -- End function
+  .section  .AMDGPU.csdata
+; Function info:
+; codeLenInByte = 32
+; NumSgprs: 36
+; NumVgprs: 2
+; ScratchSize: 0
+; MemoryBound: 0
+  .section  ".note.GNU-stack"
+  .amd_amdgpu_isa "amdgcn-unknown---gfx900"

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/amdgpu_no_merge_comments-O3.s

+  .text
+  .section  .AMDGPU.config
+  .long 47176
+  .long 0
+  .long 47180
+  .long 0
+  .long 47200
+  .long 0
+  .long 4
+  .long 0
+  .long 8
+  .long 0
+  .text
+  .hidden main                            ; -- Begin function main
+  .globl  main
+  .p2align  2
+  .type main,@function
+main:                                   ; @main
+; %bb.0:
+  s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+  v_add_u32_e32 v1, v0, v0
+  v_mul_lo_u32 v0, v1, v0
+  v_sub_u32_e32 v0, v0, v1
+  s_setpc_b64 s[30:31]
+.Lfunc_end0:
+  .size main, .Lfunc_end0-main
+                                        ; -- End function
+  .section  .AMDGPU.csdata
+; Function info:
+; codeLenInByte = 24
+; NumSgprs: 36
+; NumVgprs: 2
+; ScratchSize: 0
+; MemoryBound: 0
+  .section  ".note.GNU-stack"
+  .amd_amdgpu_isa "amdgcn-unknown---gfx900"

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/amdgpu_no_merge_comments.ll

-; RUN: llc -O0 -mtriple=amdgcn- -mcpu=gfx900 < %s | FileCheck -check-prefixes=GCN,GFX9-O0 %s
-; RUN: llc -mtriple=amdgcn- -mcpu=gfx900 < %s | FileCheck -check-prefixes=GCN,GFX9-O3 %s
+; llc is replaced with cat, we just simulate llc by printing text from these files
+; RUN: llc %S/amdgpu_no_merge_comments-O0.s | FileCheck -check-prefixes=GCN,GFX9-O0 %s
+; RUN: llc %S/amdgpu_no_merge_comments-O3.s | FileCheck -check-prefixes=GCN,GFX9-O3 %s
+
+target triple = "amdgcn--"
 
 define hidden i32 @main(i32 %a) {
   %add = add i32 %a, %a
   %mul = mul i32 %add, %a
   %sub = sub i32 %mul, %add
   ret i32 %sub
 }

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/amdgpu_no_merge_comments.ll.expected

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -O0 -mtriple=amdgcn- -mcpu=gfx900 < %s | FileCheck -check-prefixes=GCN,GFX9-O0 %s
-; RUN: llc -mtriple=amdgcn- -mcpu=gfx900 < %s | FileCheck -check-prefixes=GCN,GFX9-O3 %s
+; llc is replaced with cat, we just simulate llc by printing text from these files
+; RUN: llc %S/amdgpu_no_merge_comments-O0.s | FileCheck -check-prefixes=GCN,GFX9-O0 %s
+; RUN: llc %S/amdgpu_no_merge_comments-O3.s | FileCheck -check-prefixes=GCN,GFX9-O3 %s
+
+target triple = "amdgcn--"
 
 define hidden i32 @main(i32 %a) {
 ; GFX9-O0-LABEL: main:
 ; GFX9-O0:       ; %bb.0:
 ; GFX9-O0-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GFX9-O0-NEXT:    v_add_u32_e64 v1, v0, v0
 ; GFX9-O0-NEXT:    ; implicit-def: $sgpr4
 ; GFX9-O0-NEXT:    ; implicit-def: $sgpr4

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/amdgpu-no-merge-comments.test

 # REQUIRES: amdgpu-registered-target
 ## Check that functions with different IR comments in the output are not merged
 
-# RUN: cp -f %S/Inputs/amdgpu_no_merge_comments.ll %t.ll && %update_llc_test_checks %t.ll
+# Replace llc with cat, so we can simulate hypothetical output without actually running llc.
+# Copy the simulated output to the temporary directory.
+# 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

llvm/utils/UpdateTestChecks/common.py

def should_add_line_to_output(input_line, prefix_set, skip_global_checks = False, comment_marker = ';'):
   if m and m.group(1) in prefix_set:
     if skip_global_checks:
       global_ir_value_re = re.compile('\[\[', flags=(re.M))
       return not global_ir_value_re.search(input_line)
     return False
 
   return True
 
+# Perform lit-like substitutions
+def getSubstitutions(sourcepath):
+  sourcedir = os.path.dirname(sourcepath)
+  return [('%s', sourcepath),
+          ('%S', sourcedir),
+          ('%p', sourcedir),
+          ('%{pathsep}', os.pathsep)]
+
+def applySubstitutions(s, substitutions):
+  for a,b in substitutions:
+    s = s.replace(a, b)
+  return s
+
 # Invoke the tool that is being tested.
 def invoke_tool(exe, cmd_args, ir, preprocess_cmd=None, verbose=False):
   with open(ir) as ir_file:
+    substitutions = getSubstitutions(ir)
+
     # TODO Remove the str form which is used by update_test_checks.py and
     # update_llc_test_checks.py
     # The safer list form is used by update_cc_test_checks.py
     if preprocess_cmd:
       # Allow pre-processing the IR file (e.g. using sed):
       assert isinstance(preprocess_cmd, str)  # TODO: use a list instead of using shell
-      preprocess_cmd = preprocess_cmd.replace('%s', ir).strip()
+      preprocess_cmd = applySubstitutions(preprocess_cmd, substitutions).strip()
       if verbose:
         print('Pre-processing input file: ', ir, " with command '",
               preprocess_cmd, "'", sep="", file=sys.stderr)
       # Python 2.7 doesn't have subprocess.DEVNULL:
       with open(os.devnull, 'w') as devnull:
         pp = subprocess.Popen(preprocess_cmd, shell=True, stdin=devnull,
                               stdout=subprocess.PIPE)
         ir_file = pp.stdout
+
     if isinstance(cmd_args, list):
-      stdout = subprocess.check_output([exe] + cmd_args, stdin=ir_file)
+      args = [applySubstitutions(a, substitutions) for a in cmd_args]
+      stdout = subprocess.check_output([exe] + args, stdin=ir_file)
     else:
-      stdout = subprocess.check_output(exe + ' ' + cmd_args,
+      stdout = subprocess.check_output(exe + ' ' + applySubstitutions(cmd_args, substitutions),
                                        shell=True, stdin=ir_file)
     if sys.version_info[0] > 2:
       stdout = stdout.decode()
   # Fix line endings to unix CR style.
   return stdout.replace('\r\n', '\n')
 
 ##### LLVM IR parser
 RUN_LINE_RE = re.compile(r'^\s*(?://|[;#])\s*RUN:\s*(.*)$')