[Utils] Do not remove comments in llc test script

[sebastian-ne]

Summary

When checking if two prefixes can be merged for a function,
update_llc_test_checks.py removed IR comments before comparing
llc outputs of different RUN lines.
This means, if one RUN line emited lines starting with ';' and another
RUN line emited the same lines except the ones starting with ';', both
RUNs would be merged (if they share a prefix).

However, CHECK-NEXT lines check the comments, otherwise they fail, so
the script should not merge RUNs if they contain different comments.

[lebedev.ri]

Tests missing

[sebastian-ne]

Add test

[ggeorgakoudis]

I'm trying to understand the functionality you want to achieve. Doesn't using different prefixes avoid merging already? The test does not seem to exercise your extensions.

[sebastian-ne]

In D101312#2729394, @ggeorgakoudis wrote:

I'm trying to understand the functionality you want to achieve. Doesn't using different prefixes avoid merging already? The test does not seem to exercise your extensions.

He, that is a good point, it was my intention to add a common prefix to the test. I added that and checked that the test now fails without the script changes.

(It will end up merging the output for the two run lines. Running FileCheck will then fail for the GFX9-O0 case, because the comments are missing in the CHECK-NEXT lines.)

[sebastian-ne]

friendly ping for review

[foad]

llvm/utils/UpdateTestChecks/common.py

271

It seems like most of the "arg name scrubbing" that this function does is not relevant for asm? Perhaps update_llc_test_checks could use a much much simpler form of this function instead?

[foad]

Also, I don't understand why it would be OK for non-asm dialects to remove a whole-line comment. Maybe the problem is that SCRUB_IR_COMMENT should remove a trailing comment only, but not completely delete a whole-line comment?

[sebastian-ne]

I guess it is fine for MIR tests because they only use CHECK lines instead of CHECK-NEXT lines.

[foad]

In D101312#2744068, @sebastian-ne wrote:

I guess it is fine for MIR tests because they only use CHECK lines instead of CHECK-NEXT lines.

In add_checks() it looks like non-asm dialects use CHECK-NEXT normally, but CHECK after one or more blank lines.

[sebastian-ne]

It seems like most of the "arg name scrubbing" that this function does is not relevant for asm? Perhaps update_llc_test_checks could use a much much simpler form of this function instead?

You’re right, seems like D68819 added it and it is meant for IR test. For args_and_sig, it is still done (whatever that contains in llc tests), but for the body, the scrubbing with these regexes is skipped: https://github.com/llvm/llvm-project/blob/bbcbf21ae60c928e07dde6a1c468763b3209d1e6/llvm/utils/UpdateTestChecks/common.py#L679-L687
So, I hope it works if we do the same in this check function as when emitting the check lines.

Maybe the problem is that SCRUB_IR_COMMENT should remove a trailing comment only, but not completely delete a whole-line comment?

Probably, that is a different problem and should be fixed in a different patch (if is_asm is true, the regexes are not applied at all; IR comments are left in place for llc tests). The problem is that \s in the regex also matches—and removes—newlines. I guess the most difficult part is coming up with a testcase.

Sorry, it took me a while to look at this again.

[sebastian-ne]

Fix missing 'not' in comment.

[sebastian-ne]

friendly ping for review

[sebastian-ne]

friendly ping for review

[foad]

Looks OK to me, thanks.

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
+
+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
+
+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
+; GFX9-O0-NEXT:    v_mul_lo_u32 v0, v1, v0
+; GFX9-O0-NEXT:    v_sub_u32_e64 v0, v0, v1
+; GFX9-O0-NEXT:    s_setpc_b64 s[30:31]
+;
+; GFX9-O3-LABEL: main:
+; GFX9-O3:       ; %bb.0:
+; GFX9-O3-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GFX9-O3-NEXT:    v_add_u32_e32 v1, v0, v0
+; GFX9-O3-NEXT:    v_mul_lo_u32 v0, v1, v0
+; GFX9-O3-NEXT:    v_sub_u32_e32 v0, v0, v1
+; GFX9-O3-NEXT:    s_setpc_b64 s[30:31]
+  %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/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
+# RUN: diff -u %S/Inputs/amdgpu_no_merge_comments.ll.expected %t.ll

llvm/utils/UpdateTestChecks/common.py

 
 # Build up a dictionary of all the function bodies.
 class function_body(object):
   def __init__(self, string, extra, args_and_sig, attrs):
     self.scrub = string
     self.extrascrub = extra
     self.args_and_sig = args_and_sig
     self.attrs = attrs
-  def is_same_except_arg_names(self, extrascrub, args_and_sig, attrs):
+  def is_same_except_arg_names(self, extrascrub, args_and_sig, attrs, is_asm):
     arg_names = set()
     def drop_arg_names(match):
         arg_names.add(match.group(variable_group_in_ir_value_match))
         if match.group(attribute_group_in_ir_value_match):
             attr = match.group(attribute_group_in_ir_value_match)
         else:
             attr = ''
         return match.group(1) + attr + match.group(match.lastindex)
     def repl_arg_names(match):
         if match.group(variable_group_in_ir_value_match) is not None and match.group(variable_group_in_ir_value_match) in arg_names:
             return match.group(1) + match.group(match.lastindex)
         return match.group(1) + match.group(2) + match.group(match.lastindex)
     if self.attrs != attrs:
       return False
     ans0 = IR_VALUE_RE.sub(drop_arg_names, self.args_and_sig)
     ans1 = IR_VALUE_RE.sub(drop_arg_names, args_and_sig)
     if ans0 != ans1:
         return False
+    if is_asm:
+        # Check without replacements, the replacements are not applied to the
+        # body for asm checks.
+        return self.extrascrub == extrascrub
+
     es0 = IR_VALUE_RE.sub(repl_arg_names, self.extrascrub)
     es1 = IR_VALUE_RE.sub(repl_arg_names, extrascrub)
     es0 = SCRUB_IR_COMMENT_RE.sub(r'', es0)
     es1 = SCRUB_IR_COMMENT_RE.sub(r'', es1)
     return es0 == es1
 
   def __str__(self):
     return self.scrub
def finish_and_get_func_dict(self):
     return self._func_dict
 
   def func_order(self):
     return self._func_order
 
   def global_var_dict(self):
     return self._global_var_dict
 
-  def process_run_line(self, function_re, scrubber, raw_tool_output, prefixes):
+  def process_run_line(self, function_re, scrubber, raw_tool_output, prefixes, is_asm):
     build_global_values_dictionary(self._global_var_dict, raw_tool_output, prefixes)
     for m in function_re.finditer(raw_tool_output):
       if not m:
         continue
       func = m.group('func')
       body = m.group('body')
       attrs = m.group('attrs') if self._check_attributes else ''
       # Determine if we print arguments, the opening brace, or nothing after the
for m in function_re.finditer(raw_tool_output):
           if (self._func_dict[prefix][func] is None or
               str(self._func_dict[prefix][func]) != scrubbed_body or
               self._func_dict[prefix][func].args_and_sig != args_and_sig or
                   self._func_dict[prefix][func].attrs != attrs):
             if (self._func_dict[prefix][func] is not None and
                 self._func_dict[prefix][func].is_same_except_arg_names(
                 scrubbed_extra,
                 args_and_sig,
-                attrs)):
+                attrs,
+                is_asm)):
               self._func_dict[prefix][func].scrub = scrubbed_extra
               self._func_dict[prefix][func].args_and_sig = args_and_sig
               continue
             else:
               # This means a previous RUN line produced a body for this function
               # that is different from the one produced by this current RUN line,
               # so the body can't be common accross RUN lines. We use None to
               # indicate that.

llvm/utils/update_analyze_test_checks.py

for prefixes, opt_args in prefix_list:
       common.debug('Extracted opt cmd:', opt_basename, opt_args, file=sys.stderr)
       common.debug('Extracted FileCheck prefixes:', str(prefixes), file=sys.stderr)
 
       raw_tool_outputs = common.invoke_tool(args.opt_binary, opt_args, test)
 
       # Split analysis outputs by "Printing analysis " declarations.
       for raw_tool_output in re.split(r'Printing analysis ', raw_tool_outputs):
         builder.process_run_line(common.ANALYZE_FUNCTION_RE, common.scrub_body,
-                                 raw_tool_output, prefixes)
+                                 raw_tool_output, prefixes, False)
 
     func_dict = builder.finish_and_get_func_dict()
     is_in_function = False
     is_in_function_start = False
     prefix_set = set([prefix for prefixes, _ in prefix_list for prefix in prefixes])
     common.debug('Rewriting FileCheck prefixes:', str(prefix_set), file=sys.stderr)
     output_lines = []
     output_lines.append(autogenerated_note)

llvm/utils/update_cc_test_checks.py

with tempfile.NamedTemporaryFile() as f:
                 'Please specify --llvm-bin or --opt', file=sys.stderr)
           sys.exit(1)
         extra_args[0] = args.opt
       raw_tool_output = common.invoke_tool(extra_args[0],
                                            extra_args[1:], f.name)
   if '-emit-llvm' in clang_args:
     builder.process_run_line(
             common.OPT_FUNCTION_RE, common.scrub_body, raw_tool_output,
-            prefixes)
+            prefixes, False)
   else:
     print('The clang command line should include -emit-llvm as asm tests '
           'are discouraged in Clang testsuite.', file=sys.stderr)
     sys.exit(1)
 
 def exec_run_line(exe):
   popen = subprocess.Popen(exe, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
   stdout, stderr = popen.communicate()

llvm/utils/update_llc_test_checks.py

for prefixes, llc_tool, llc_args, preprocess_cmd, triple_in_cmd, march_in_cmd in run_list:
       raw_tool_output = common.invoke_tool(ti.args.llc_binary or llc_tool,
                                            llc_args, ti.path, preprocess_cmd,
                                            verbose=ti.args.verbose)
       triple = triple_in_cmd or triple_in_ir
       if not triple:
         triple = asm.get_triple_from_march(march_in_cmd)
 
       scrubber, function_re = asm.get_run_handler(triple)
-      builder.process_run_line(function_re, scrubber, raw_tool_output, prefixes)
+      builder.process_run_line(function_re, scrubber, raw_tool_output, prefixes, True)
 
     func_dict = builder.finish_and_get_func_dict()
 
     is_in_function = False
     is_in_function_start = False
     func_name = None
     prefix_set = set([prefix for p in run_list for prefix in p[0]])
     common.debug('Rewriting FileCheck prefixes:', str(prefix_set))

llvm/utils/update_test_checks.py

for ti in common.itertests(initial_args.tests, parser,
     for prefixes, opt_args, preprocess_cmd in prefix_list:
       common.debug('Extracted opt cmd: ' + opt_basename + ' ' + opt_args)
       common.debug('Extracted FileCheck prefixes: ' + str(prefixes))
 
       raw_tool_output = common.invoke_tool(ti.args.opt_binary, opt_args,
                                            ti.path, preprocess_cmd=preprocess_cmd,
                                            verbose=ti.args.verbose)
       builder.process_run_line(common.OPT_FUNCTION_RE, common.scrub_body,
-              raw_tool_output, prefixes)
+              raw_tool_output, prefixes, False)
 
     func_dict = builder.finish_and_get_func_dict()
     is_in_function = False
     is_in_function_start = False
     has_checked_pre_function_globals = False
     prefix_set = set([prefix for prefixes, _, _ in prefix_list for prefix in prefixes])
     common.debug('Rewriting FileCheck prefixes:', str(prefix_set))
     output_lines = []