update_mir_test_checks.py doesn't separate different prefix checks #63112

[eddiep24]

Summary

I looked through the update_llc_test_checks.py to see how the spacing is handled between prefixes. In the update_llc_test_checks.py case, the common.py file is called. More specifically, after every prefix group, the following line adds a space between comments:

if is_backend:

if len(printed_prefixes) != 0:
       output_lines.append(comment_marker)

After looking at update_mir_test_checks.py, I saw that the following three lines were commented out:

if printed_prefixes:

1. Add some space between different check prefixes. output_lines.append('')

After uncommenting the code and running the code on several test files, the comment indeed indicated the correct purpose of the code.

[RKSimon]

Please can you include the diff on some test files - the llvm-project\llvm\test\CodeGen\X86\GlobalISel\legalize-add* files might be a good example

[RKSimon]

By the looks of it repeatedly running the script on a file adds newlines at the top of each function's set of checks? These shouldn't occur

llvm/utils/update_mir_test_checks.py

240

ideally this would be an indented ';' and not just an empty line

[eddiep24]

This comment was removed by eddiep24.

[eddiep24]

The lines at the top were being made because the should_add_line_to_output(input_line, prefix_set) function was skipping check lines but not the comments. Thus, the comments were getting pushed to the top. My solution was to ignore comments in the same fashion and use the previous line in the output_lines array to identify the correct indentation. Thanks for the feedback on the previous diff :). Attached are the diffs for the tests you recommended. They worked out nicely.

legalize-add-v256.diff13 KBDownload

legalize-add-v512.diff20 KBDownload

legalize-add.diff3 KBDownload

[RKSimon]

Thanks @eddiep24 - that works for me

Do you need to update llvm-project\llvm\test\tools\UpdateTestChecks\update_mir_test_checks\Inputs\x86-multiple-prefixes.ll.expected ? I think it now needs a ';' line in the zero128() test?

[RKSimon]

@eddiep24 reverse-ping?

[eddiep24]

Hey Simon, thanks for the heads up. I ran the script and inserted the comment where it needed to go.

[RKSimon]

LGTM, cheers - if you don't have commit access please post your full name / email here and I'll push it for you.

[eddiep24]

Awesome:
Eddie Phillips ([e-mail])

llvm/test/tools/UpdateTestChecks/update_mir_test_checks/Inputs/x86-multiple-prefixes.ll.expected

 
 define void @zero128() nounwind ssp {
   ; NOAVX-LABEL: name: zero128
   ; NOAVX: bb.0 (%ir-block.0):
   ; NOAVX-NEXT:   [[V_SET0_:%[0-9]+]]:vr128 = V_SET0
   ; NOAVX-NEXT:   [[MOV64rm:%[0-9]+]]:gr64 = MOV64rm $rip, 1, $noreg, target-flags(x86-gotpcrel) @z, $noreg :: (load (s64) from got, align 16)
   ; NOAVX-NEXT:   MOVAPSmr killed [[MOV64rm]], 1, $noreg, 0, $noreg, killed [[V_SET0_]] :: (store (s128) into @z)
   ; NOAVX-NEXT:   RET 0
+  ;
   ; AVX-LABEL: name: zero128
   ; AVX: bb.0 (%ir-block.0):
   ; AVX-NEXT:   [[V_SET0_:%[0-9]+]]:vr128 = V_SET0
   ; AVX-NEXT:   [[MOV64rm:%[0-9]+]]:gr64 = MOV64rm $rip, 1, $noreg, target-flags(x86-gotpcrel) @z, $noreg :: (load (s64) from got, align 16)
   ; AVX-NEXT:   VMOVAPSmr killed [[MOV64rm]], 1, $noreg, 0, $noreg, killed [[V_SET0_]] :: (store (s128) into @z)
   ; AVX-NEXT:   RET 0
   store <4 x float> zeroinitializer, ptr @z, align 16
   ret void
 }
 

llvm/utils/update_mir_test_checks.py

 ):
     printed_prefixes = set()
     for run in run_list:
         for prefix in run.prefixes:
             if prefix in printed_prefixes:
                 break
             if not func_dict[prefix][func_name]:
                 continue
-            # if printed_prefixes:
-            #     # Add some space between different check prefixes.
-            #     output_lines.append('')
+            if printed_prefixes:
+                # Add some space between different check prefixes.
+                indent = len(output_lines[-1]) - len(output_lines[-1].lstrip(" "))
+                output_lines.append(" "*indent + ";")
             printed_prefixes.add(prefix)
             log("Adding {} lines for {}".format(prefix, func_name), args.verbose)
             add_check_lines(
                 test,
                 output_lines,
                 prefix,
                 func_name,
                 single_bb,
for name in current_names:
         if name.rstrip("0123456789") == base:
             i += 1
     if i:
         return "{}{}".format(base, i)
     return base
 
 
 def should_add_line_to_output(input_line, prefix_set):
-    # Skip any check lines that we're handling.
+    # Skip any check lines that we're handling as well as comments
     m = common.CHECK_RE.match(input_line)
-    if m and m.group(1) in prefix_set:
+    if (m and m.group(1) in prefix_set) or re.search("^[ \t]*;", input_line):
         return False
     return True
 
 
 def update_test_file(args, test, autogenerated_note):
     with open(test) as fd:
         input_lines = [l.rstrip() for l in fd]