[UTC] Add --version argument

[nikic]

Summary

We have a number of pending changes to update_test_checks.py (and friends) that are essentially blocked on test churn: If the output of UTC for an existing flag combination changes, then the next time a test is regenerated, it will contain many spurious changes. This makes changes to UTC default behavior essentially impossible.

Examples of such changes are:

• D133943/D142373 want --function-signature to also check the return type/attributes.
• D139006/D140212 want to make --function-signature the default behavior.
• D142452 wants to add wildcards for block labels.

This patch tries to resolve this issue by adding a --version argument, which works as follows:

• When regenerating an old test, the default version is 1.
• When generating a new test, the default version is the newest.
• When an explicit version is specified, that of course wins.

This means that any currently existing tests will keep using --version 1 format, while any new tests will automatically embed --version 2 (or whatever the latest is), and then keep using that test format from then on.

[nikic]

Add missing test file.

[asb]

My initial reaction is that this is a nice solution to the problem - thank you! The main practical challenge I see is the the one you suggest in the summary - that it might not be easy to decide when it's "worth" introducing a new version, and there might be pressure in practice to group together multiple changes at once.

I'm interested to hear what others think.

[nhaehnle]

A couple of thoughts, not really having a fully-formed opinion on this yet:

• This is a nice trick to avoid the problem of test churn.
• I worry about the complexity of maintaining the different versions.
• In the past, I have on occasion made commits that only re-run UTC on existing tests, without making any other changes, and without posting them for review.
  • I added update_any_test_checks.py in part to make that sort of change easier (though the primary motivation was to just update tests after code changes)
  • I wonder if / to what extent normalizing and perhaps automating this process is an alternative solution.

[jsilvanus]

I would not be so worried about the complexity of having to maintain multiple version formats -- we can think of the version flag as setting defaults for command-line options that alter the test format.
We already have such options (e.g. mentioned --function-signature), so this is not new in principle, it just provides a convenient means to set these.

On the implementation side, I'm wondering whether it would be better to avoid directly checking version in the implementation, and instead derive some options (similar to args, or even args itself?) from the version and then only use that?
For example, if some old version introduced several changes (to avoid "version churn"), it is later probably easier to later remove support for one of them is there is a dedicated internal flag for each change.

[asb]

In D142473#4077336, @nhaehnle wrote:

• In the past, I have on occasion made commits that only re-run UTC on existing tests, without making any other changes, and without posting them for review.
  • I added update_any_test_checks.py in part to make that sort of change easier (though the primary motivation was to just update tests after code changes)
  • I wonder if / to what extent normalizing and perhaps automating this process is an alternative solution.

This is a good point, and worrying less about churn to existing tests is I think not a bad option. It's natural to want to compare such changes to other automated changes like code reformatting, but I think in practice it's very different - as the only changes are to tool-generated lines, it doesn't really cause the same issues with merge conflicts.

[nikic]

In D142473#4077309, @asb wrote:

My initial reaction is that this is a nice solution to the problem - thank you! The main practical challenge I see is the the one you suggest in the summary - that it might not be easy to decide when it's "worth" introducing a new version, and there might be pressure in practice to group together multiple changes at once.

I think from a maintenance perspective, it actually makes little difference whether these are in one version or in three. I believe the implementation of each change is essentially orthogonal, and would not become substantially simpler (or simpler at all?) when combined in one version. So this would mostly be a cosmetic question for me.

Maybe worth mentioning that I don't think every change to UTC output needs a version bump: Only those that could cause significant churn. For example, I don't think we should bump the version every time we add support for recognizing some new metadata, or other changes that don't affect many tests.

In D142473#4077336, @nhaehnle wrote:

• In the past, I have on occasion made commits that only re-run UTC on existing tests, without making any other changes, and without posting them for review.

Yes, this isn't an unusual thing to do. Generally if you either hit a test that hasn't been converted yet, or there have been some substantial changes to instruction names. It's mostly a question of scale. At this point, with most tests (that I care about) using UTC, this is something that one has to do only occasionally. With changes like "enable --function-signature by default" you have to do that for literally every single test you touch.

Marginally increased maintenance costs in UTC are something that affects only few people and few changes, while test churn affects everyone. The "regenerate test checks, see that some need a regenerated baseline, checkout main, rebuild, regenerate baseline, push the change, rebase, find out that somebody 'improved' an ADT header in the meantime, wait 15 minutes for the build to finish, go back to your actual work" cycle adds non-trivial overhead to development, and test management is already a big time-sink as it is.

[arichardson]

I like this approach, it's a lot less hacky than my attempt to default function signatures to on. I'll rebase that to check for version>1 instead.

I'm not sure if this should include the change to match full lines though, I think that should be a separate commit (but the implementation looks fine to me).

[nikic]

Only add the --version flag, don't implement any behavior change in this revision. As such, the default version stays at 1 and this is a no-op for now.

Future changes should bump the DEFAULT_VERSION constant and add a changelog entry.

[jsilvanus]

llvm/utils/UpdateTestChecks/common.py

24

Maybe mention that this is equivalent to no version specified.

148

default=DEFAULT_VERSION?

[nikic]

Additional comments

[nikic]

llvm/utils/UpdateTestChecks/common.py

148

This one should always stay at 1, added a comment to explain.

[jsilvanus]

llvm/utils/UpdateTestChecks/common.py

148

Makes sense, thanks.

[bogner]

Multiple people had concerns with the added complexity of having to maintain historical versions of behaviour here (a concern which I share), and absolutely none of them accepted the revision or commented after the comment that you don’t think it’ll be too bad in practice (with little explanation as to why).

I really don’t think it was appropriate to commit this without further discussion here.

[nikic]

Sorry, I did not interpret those comments as blocking concerns. Happy to revert this if they were intended as such -- this is effectively an NFC patch until DEFAULT_VERSION gets bumped, so we can still backtrack to a different approach (though I'm not aware of any that would have less maintenance burden).

clang/test/utils/update_cc_test_checks/lit.local.cfg

else:
 
     config.test_format = lit.formats.ShTest(execute_external=False)
     config.suffixes = ['.test']
 
     clang_path = os.path.join(config.clang_tools_dir, 'clang')
     extra_args = '--clang ' + shell_quote(clang_path)
     opt_path = os.path.join(config.llvm_tools_dir, 'opt')
     extra_args += ' --opt ' + shell_quote(opt_path)
+    # Specify an explicit default version in UTC tests, so that the --version
+    # embedded in UTC_ARGS does not change in all test expectations every time
+    # the default is bumped.
+    extra_args += ' --version=1'
     script_path = os.path.join(config.llvm_src_root, 'utils',
                                'update_cc_test_checks.py')
     assert os.path.isfile(script_path)
     # Windows: llvm-lit.py, Linux: llvm-lit
     if config.llvm_external_lit:
         lit = config.llvm_external_lit
     else:
         lit = shell_quote(glob.glob(os.path.join(config.llvm_tools_dir, 'llvm-lit*'))[0])

llvm/test/tools/UpdateTestChecks/lit.local.cfg

except ImportError:
     from pipes import quote as shell_quote
 
 
 def add_update_script_substition(name, python_exe=config.python_executable,
                                  extra_args=''):
     assert name.startswith('%')
     script_path = os.path.join(config.llvm_src_root, 'utils', name[1:] + '.py')
     assert os.path.isfile(script_path)
+    # Specify an explicit default version in UTC tests, so that the --version
+    # embedded in UTC_ARGS does not change in all test expectations every time
+    # the default is bumped.
+    extra_args += ' --version=1'
     config.substitutions.append(
         (name, "'%s' %s %s" % (python_exe, script_path, extra_args)))
 
 
 config.test_format = lit.formats.ShTest(execute_external=False)
 config.suffixes = ['.test']
 
 llc_path = os.path.join(config.llvm_tools_dir, 'llc')

llvm/utils/UpdateTestChecks/common.py

 from typing import List
 
 ##### Common utilities for update_*test_checks.py
 
 
 _verbose = False
 _prefix_filecheck_ir_name = ''
 
+"""
+Version changelog:
+
+1: Initial version, used by tests that don't specify --version explicitly.
+"""
+DEFAULT_VERSION = 1
+
 class Regex(object):
   """Wrap a compiled regular expression object to allow deep copy of a regexp.
   This is required for the deep copy done in do_scrub.
 
   """
   def __init__(self, regex):
     self.regex = regex
 
parser.add_argument('--global-hex-value-regex', nargs='+', default=[],
                       help='List of regular expressions such that, for matching global value declarations, literal integer values should be encoded in hex in the associated FileCheck directives')
   # FIXME: in 3.9, we can use argparse.BooleanOptionalAction. At that point,
   # we need to rename the flag to just -generate-body-for-unused-prefixes.
   parser.add_argument('--no-generate-body-for-unused-prefixes',
                       action='store_false',
                       dest='gen_unused_prefix_body',
                       default=True,
                       help='Generate a function body that always matches for unused prefixes. This is useful when unused prefixes are desired, and it avoids needing to annotate each FileCheck as allowing them.')
+  # This is the default when regenerating existing tests. The default when
+  # generating new tests is determined by DEFAULT_VERSION.
+  parser.add_argument('--version', type=int, default=1,
+                      help='The version of output format')
   args = parser.parse_args()
   global _verbose, _global_value_regex, _global_hex_value_regex
   _verbose = args.verbose
   _global_value_regex = args.global_value_regex
   _global_hex_value_regex = args.global_hex_value_regex
   return args
 
 
for pattern in test_patterns:
     # On Windows we must expand the patterns ourselves.
     tests_list = glob.glob(pattern)
     if not tests_list:
       warn("Test file pattern '%s' was not found. Ignoring it." % (pattern,))
       continue
     for test in tests_list:
       with open(test) as f:
         input_lines = [l.rstrip() for l in f]
-      args = parser.parse_args()
+      first_line = input_lines[0] if input_lines else ""
+      is_regenerate = UTC_ADVERT in first_line
+
+      # If we're generating a new test, set the default version to the latest.
+      argv = sys.argv[:]
+      if not is_regenerate:
+        argv.insert(1, '--version=' + str(DEFAULT_VERSION))
+
+      args = parser.parse_args(argv[1:])
       if argparse_callback is not None:
         argparse_callback(args)
-      argv = sys.argv[:]
-      first_line = input_lines[0] if input_lines else ""
-      if UTC_ADVERT in first_line:
+      if is_regenerate:
         if script_name not in first_line and not args.force_update:
           warn("Skipping test which wasn't autogenerated by " + script_name, test)
           continue
         args, argv = check_for_command(first_line, parser, args, argv, argparse_callback)
       elif args.update_only:
         assert UTC_ADVERT not in first_line
         warn("Skipping test which isn't autogenerated: " + test)
         continue