[Utils] Allow "on-the-fly" argument changes for update_test_check scripts

[jdoerfert]

Summary

Update test scripts were limited because they performed a single action
on the entire file and if that action was controlled by arguments, like
the one introduced in D68819, there was no record of it.

This patch introduces the capability of changing the arguments passed to
the script "on-the-fly" while processing a test file. In addition, an
"on/off" switch was added so that processing can be disabled for parts
of the file where the content is simply copied. The last extension is a
record of the invocation arguments in the auto generated NOTE. These
arguments are also picked up in a subsequent invocation, allowing
updates with special options enabled without user interaction.

To change the arguments the string UTC_ARGS: has to be present in a
line, followed by "additional command line arguments". That is
everything that follows UTC_ARGS: will be added to a growing list
of "command line arguments" which is reparsed after every update.

[arichardson]

This seems useful, thanks! Another case that would be nice is if there was a way to keep comments inside the autogenerated CHECK lines.

For example:

; CHECK-NEXT: ...
;; We generate a foo instruction and not a bar instruction here because...
; CHECK-NEXT: foo a, b, c
; CHECK-NEXT: ...

Right now if you re-run the update script the comment will be moved after all the CHECK lines. I wonder if there is a way to keep inline comments (maybe only if the CHECK: lines before and after didn't change?). Would this work with the UTC_ARGS: --turn off method?

llvm/utils/UpdateTestChecks/common.py

53

I think you need a r for the second string as well since otherwise newer python will complain that \s is not a valid escape sequence.

[davezarzycki]

In D69701#1729829, @arichardson wrote:

This seems useful, thanks! Another case that would be nice is if there was a way to keep comments inside the autogenerated CHECK lines.

For example:

; CHECK-NEXT: ...
;; We generate a foo instruction and not a bar instruction here because...
; CHECK-NEXT: foo a, b, c
; CHECK-NEXT: ...

Right now if you re-run the update script the comment will be moved after all the CHECK lines. I wonder if there is a way to keep inline comments (maybe only if the CHECK: lines before and after didn't change?). Would this work with the UTC_ARGS: --turn off method?

IMHO, that kind of comment belongs with the code that *generates* the output, rather than buried in the middle of a random test file. Separably, if it isn't clear what the test is testing, then perhaps some discussion at the top of the file/function seems appropriate, perhaps with links back to the relevant files/functions that are being tested. For example: "Please see the discussion inside of function foo in Bar.cpp for what is being tested below."

[jdoerfert]

In D69701#1729845, @davezarzycki wrote:

In D69701#1729829, @arichardson wrote:

This seems useful, thanks! Another case that would be nice is if there was a way to keep comments inside the autogenerated CHECK lines.

For example:

; CHECK-NEXT: ...
;; We generate a foo instruction and not a bar instruction here because...
; CHECK-NEXT: foo a, b, c
; CHECK-NEXT: ...

Right now if you re-run the update script the comment will be moved after all the CHECK lines. I wonder if there is a way to keep inline comments (maybe only if the CHECK: lines before and after didn't change?). Would this work with the UTC_ARGS: --turn off method?

It would not. The generation still happens in bulk. See below.

IMHO, that kind of comment belongs with the code that *generates* the output, rather than buried in the middle of a random test file. Separably, if it isn't clear what the test is testing, then perhaps some discussion at the top of the file/function seems appropriate, perhaps with links back to the relevant files/functions that are being tested. For example: "Please see the discussion inside of function foo in Bar.cpp for what is being tested below."

There are multiple use cases here for sure and the script addresses only one:
You generate check lines with the script, then you modify some of them for some reason. (Or you insert a test with manual check in lines into a file that has automatically generated ones.)
With this change you can "protect" the manual check lines in a reasonable way so we can rerun the script on files with the NOTE line.

llvm/utils/UpdateTestChecks/common.py

53

agreed

[jdoerfert]

Lessons learned from actually using it (D68766)

[jdoerfert]

Any comments (on this and the other [utils] patches)?

[lebedev.ri]

Now that i believe there's some testing infra for these utils, add/update tests for this?

[jdoerfert]

Add and adjust regression tests

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

[lebedev.ri]

This seems ok, but i don't feel confident accepting.
Any takers?

[arichardson]

llvm/utils/update_test_checks.py

73

I find the equals in the flag slightly confusing/non-intuitive.

Maybe better to add a action=store_true and a action=store_false booleanoption with the same dest=. Maybe --turn-on/--turn-off or simply --enable/--disable?

192

Use a bool argparse option?

[arichardson]

If I understand this correctly existing check lines between off/on directives should be copied verbatim. Could you add one (that does not match the auto-generated content) to the disabled part of the test?

[jdoerfert]

In D69701#1799763, @arichardson wrote:

If I understand this correctly existing check lines between off/on directives should be copied verbatim. Could you add one (that does not match the auto-generated content) to the disabled part of the test?

Will do.

Maybe better to add a action=store_true and a action=store_false booleanoption with the same dest=. Maybe --turn-on/--turn-off or simply --enable/--disable?

I'll look into that and update the test.

[lebedev.ri]

Needs rebasing

[jdoerfert]

Rebase, make the arguments -enable and -disable

[jdoerfert]

Add check lines in the "disabled" part to show how they "survive"

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[jdoerfert]

ping

[lebedev.ri]

@MaskRay thoughts?

[arichardson]

Looks good to me. Since this is in common.py, it would be nice to see it also applied to update_cc_test checks or update_llc_test_checks.

llvm/test/tools/UpdateTestChecks/update_test_checks/basic.test

11 ↗

(On Diff #241198)

Maybe expand this comment to include "since the generated file will have --function-signature in an UTC_ARGS: comment" or similar

[jdoerfert]

In D69701#1869559, @arichardson wrote:

Looks good to me. Since this is in common.py, it would be nice to see it also applied to update_cc_test checks or update_llc_test_checks.

I agree but will leave that as a future improvement ;)

llvm/test/tools/UpdateTestChecks/update_test_checks/basic.test

11 ↗

(On Diff #241198)

Will do.

llvm/utils/UpdateTestChecks/common.py

def invoke_tool(exe, cmd_args, ir):
   return stdout.replace('\r\n', '\n')
 
 ##### LLVM IR parser
 
 RUN_LINE_RE = re.compile(r'^\s*[;#]\s*RUN:\s*(.*)$')
 CHECK_PREFIX_RE = re.compile(r'--?check-prefix(?:es)?[= ](\S+)')
 PREFIX_RE = re.compile('^[a-zA-Z0-9_-]+$')
 CHECK_RE = re.compile(r'^\s*[;#]\s*([^:]+?)(?:-NEXT|-NOT|-DAG|-LABEL|-SAME)?:')
+UTC_ARGS_KEY = 'UTC_ARGS:'
+UTC_ARGS_CMD = re.compile(r'.*' + UTC_ARGS_KEY + '\s*(?P<cmd>.*)\s*$')
 
 OPT_FUNCTION_RE = re.compile(
     r'^\s*define\s+(?:internal\s+)?[^@]*@(?P<func>[\w-]+?)\s*'
     r'(?P<args_and_sig>\((\)|(.*?[\w\.\-]+?)\))[^{]*)\{\n(?P<body>.*?)^\}$',
     flags=(re.M | re.S))
 
 ANALYZE_FUNCTION_RE = re.compile(
     r'^\s*\'(?P<analysis>[\w\s-]+?)\'\s+for\s+function\s+\'(?P<func>[\w-]+?)\':'
if "check-prefix=" in part:
       prefix = part.split('=', 1)[1]
       check_prefix(prefix)
     elif "check-prefixes=" in part:
       prefixes = part.split('=', 1)[1].split(',')
       for prefix in prefixes:
         check_prefix(prefix)
         if prefixes.count(prefix) > 1:
           warn("Supplied prefix '%s' is not unique in the prefix list." % (prefix,))
+
+def get_autogennote_suffix(parser, args):
+    autogenerated_note_args = ''
+    for k, v in args._get_kwargs():
+        if parser.get_default(k) == v or k == 'tests' or k == 'update_only':
+            continue
+        k = k.replace('_', '-')
+        if type(v) is bool:
+            autogenerated_note_args += '--%s ' % (k)
+        else:
+            autogenerated_note_args += '--%s %s ' % (k, v)
+    if autogenerated_note_args:
+        autogenerated_note_args = ' %s %s' % (UTC_ARGS_KEY, autogenerated_note_args)
+    return autogenerated_note_args
+
+
+def check_for_command(line, parser, args, argv):
+    cmd_m = UTC_ARGS_CMD.match(line)
+    if cmd_m:
+        cmd = cmd_m.group('cmd').strip().split(' ')
+        argv += cmd
+        args = parser.parse_args(argv)
+    return args

llvm/utils/update_test_checks.py

def main():
   parser.add_argument('-u', '--update-only', action='store_true',
                       help='Only update test if it was already autogened')
   parser.add_argument('-p', '--preserve-names', action='store_true',
                       help='Do not scrub IR names')
   parser.add_argument('--function-signature', action='store_true',
                       help='Keep function signature information around for the check line')
   parser.add_argument('--scrub-attributes', action='store_true',
                       help='Remove attribute annotations (#0) from the end of check line')
+  parser.add_argument('--turn', choices=['on', 'off'], default='on',
+                      help='Initial state, check lines are upded only in "on" state')
   parser.add_argument('tests', nargs='+')
   args = parser.parse_args()
 
   script_name = os.path.basename(__file__)
   autogenerated_note = (ADVERT + 'utils/' + script_name)
 
   opt_basename = os.path.basename(args.opt_binary)
   if not re.match(r'^opt(-\d+)?$', opt_basename):
     common.error('Unexpected opt name: ' + opt_basename)
     sys.exit(1)
   opt_basename = 'opt'
 
   for test in args.tests:
     if not glob.glob(test):
       common.warn("Test file pattern '%s' was not found. Ignoring it." % (test,))
       continue
 
   # On Windows we must expand the patterns ourselves.
   test_paths = [test for pattern in args.tests for test in glob.glob(pattern)]
   for test in test_paths:
+    argv = sys.argv[:]
+    args = parser.parse_args()
     if args.verbose:
       print('Scanning for RUN lines in test file: ' + test, file=sys.stderr)
     with open(test) as f:
       input_lines = [l.rstrip() for l in f]
 
     first_line = input_lines[0] if input_lines else ""
     if 'autogenerated' in first_line and script_name not in first_line:
       common.warn("Skipping test which wasn't autogenerated by " + script_name, test)
       continue
+    if first_line and 'autogenerated' in first_line:
+      args = common.check_for_command(first_line, parser, args, argv)
+    test_autogenerated_note = autogenerated_note + common.get_autogennote_suffix(parser, args)
 
     if args.update_only:
       if not first_line or 'autogenerated' not in first_line:
         common.warn("Skipping test which isn't autogenerated: " + test)
         continue
 
     # If requested we scrub trailing attribute annotations, e.g., '#0', together with whitespaces
     if args.scrub_attributes:
for prefixes, opt_args in prefix_list:
               args.function_signature)
 
     is_in_function = False
     is_in_function_start = False
     prefix_set = set([prefix for prefixes, _ in prefix_list for prefix in prefixes])
     if args.verbose:
       print('Rewriting FileCheck prefixes: %s' % (prefix_set,), file=sys.stderr)
     output_lines = []
-    output_lines.append(autogenerated_note)
+    output_lines.append(test_autogenerated_note)
 
     for input_line in input_lines:
+      # Discard any previous script advertising.
+      if input_line.startswith(ADVERT):
+        continue
+
+      args = common.check_for_command(input_line, parser, args, argv)
+      if args.turn == 'off':
+          output_lines.append(input_line)
+          continue
       if is_in_function_start:
         if input_line == '':
           continue
         if input_line.lstrip().startswith(';'):
           m = common.CHECK_RE.match(input_line)
           if not m or m.group(1) not in prefix_set:
             output_lines.append(input_line)
             continue
for input_line in input_lines:
           input_line = common.SCRUB_LEADING_WHITESPACE_RE.sub(r'  ', input_line)
           output_lines.append(input_line)
         else:
           continue
         if input_line.strip() == '}':
           is_in_function = False
         continue
 
-      # Discard any previous script advertising.
-      if input_line.startswith(ADVERT):
-        continue
-
       # If it's outside a function, it just gets copied to the output.
       output_lines.append(input_line)
 
       m = IR_FUNCTION_RE.match(input_line)
       if not m:
         continue
       func_name = m.group(1)
       if args.function is not None and func_name != args.function: