[mlir] Add update_mlir_test_checks.py

[stephenneuendorffer]

Summary

mlir/utils/generate_test_checks.py is somewhat braindead about updating
tests. Recent improvements allow it to modify existing tests, but still
require writing a verbose command line which duplicates information
that exists in the RUN line of the test. This script follows the pattern
used in the rest of llvm, with some MLIR-sytax specific tweaks to generate
tests the way that generate_test_checks.py does it. I expect that this is not
perfect and could/should be reduced to use more of UpdateTestChecks/common.py

[mehdi_amini]

llvm/utils/update_mlir_test_checks.py

29

Something that I believe is important is to not just have blindly generated CHECKs but have a human editing the tests (documenting, minimizing the number of checks, etc.).

Can we have the script adding a paragraph mentioning in the actual generated test? (with a link to the testing guide, etc.)

[stephenneuendorffer]

llvm/utils/update_mlir_test_checks.py

29

Conceivably...but I'm guessing that we'd end up with the same paragraph in lots of files. I tend to use this to auto-update tests in a mechanical way which is easy to verify, but hard to do by hand. I'm specifically trying to avoid opening every file when updating lots of tests, so I think adding a bunch of text in the file that is expected to be removed would be a bad idea. What if we just added a boilerplate message about usage and norms that was generated when the script is run?

[silvas]

It's kind of confusing that this is in llvm/utils while generate-test-checks.py is in mlir/utils. Is there a plan to reconcile this difference?

[mehdi_amini]

llvm/utils/update_mlir_test_checks.py

29

Conceivably...but I'm guessing that we'd end up with the same paragraph in lots of files.

I mean: since the user is supposed to manually edit significantly the output of the tool, removing the paragraph doesn't seem much.

The challenge is not get the output of the script just good enough to be checked in as-is but leading to actual bad tests

[stephenneuendorffer]

In D83924#2156278, @silvas wrote:

It's kind of confusing that this is in llvm/utils while generate-test-checks.py is in mlir/utils. Is there a plan to reconcile this difference?

it exists in llvm/utils because there are a bunch of other similar scripts there (for llc, mir, ll, etc.) I'm open to suggestions on how to reconcile this. Personally, I would replace generate-test-checks.py with this, because I think it's easier to use in most cases, but sometimes generate-test-checks.py might be better. We could, perhaps, add an option to this that doesn't modify the test, but just generates the test checks on standard out which would enable both usage models.

[stephenneuendorffer]

llvm/utils/update_mlir_test_checks.py

29

Personally, I think we'll get better tests if we get people to focus on making good tests, rather than tedious manual editing. I think that this goes more into the input of tests, than the format of the check lines. I think forcing people to 'manually edit significantly' should definitely not be the goal.

[rriddle]

There seems to be a lot of hardcoding in here related to func. Given that func is not even the only function in upstream MLIR, and that for several dialects(existing and upcoming) func isn't even used at all, what is the story for those users?

llvm/utils/update_mlir_test_checks.py

29

Forcing lots of manual edits is not the goal, but a more important goal is not to have "diff" tests. IMO having a large amount of users manually edit is much better than having one person have to deal with the fallout of having to update hundreds of tests that broke because they were testing something unnecessary. It spreads out the maintenance efforts and makes evolution easier. (Speaking from having had to deal with this a lot in practice.)

[stephenneuendorffer]

In D83924#2156773, @rriddle wrote:

There seems to be a lot of hardcoding in here related to func. Given that func is not even the only function in upstream MLIR, and that for several dialects(existing and upcoming) func isn't even used at all, what is the story for those users?

I've been scratching my head a bit about how to build tools like this which need to understand the custom op formats. I'm also thinking of things like automatic syntax highlighting in emacs/other editors. One possibility is that this should actually be an MLIR-based tool which uses a different syntax which parses comments as ops. It would then be able to query operations and apply rules for where test boundaries should be. so instead of 'func' hardcoded as a string, the tool would query for FunctionLikeInterface. There are other things in here that are hardcoded too in regexps. The best thing I can say at the moment is that the tool just won't work in such cases.

[jdoerfert]

[Drive by] tests?

[stephenneuendorffer]

In D83924#2156968, @jdoerfert wrote:

[Drive by] tests?

Yeah, would be good idea.

[silvas]

llvm/utils/update_mlir_test_checks.py

3

mlir_opt -> mlir-opt

17

This could use an MLIR-ification

29

+1 to what River says. Encouraging some testing thoughtfulness would be really great.

If we really want to be evil we can insert some random garbage characters in the generated test checks 😈 so that people will have to manually run the test / look through the checks / test failures to remove them. And of course, the easiest way to fix such deliberately mangled tests is to delete whole CHECK lines, types, and other stuff, which is exactly what we want them to be doing; "the more I delete, the less I have to audit for random garbage inserted by the update_mlir_test_checks script".

Less evilly, I think that Mehdi's idea of generating a "test best practices summary + link to complete best practices doc" sounds good. It could look like:

PLEASE make sure that you trim down what is being checked by this test to the minimum that gives decent coverage. That will make life easier for people maintaining the test in the future.
- Does your test need to check all operations? If not, then remove the unnecessary CHECK lines! 
- Does your test need to check the types? If not, then remove the types from the CHECK lines!
- Does your test need to check all attributes of the ops? If not, then use {{.*}} to skip them!
- Does your test really need to check that SSA values are passed correctly? (rather than the mere presence of an op). If not, then remove the %[[FOO:.*]] from CHECK lines!

For more info on testing best practices see <link to more thorough docs>

Btw, does this script throw away any user edits? (haven't looked at the code very much). If it does, then users will probably get bored of re-trimming-down the autogenerated stuff no matter how much encouragement we give them.

292

why is this code commented out?

302

stray commented out code?

[stephenneuendorffer]

llvm/utils/update_mlir_test_checks.py

29

Can you help me out, because I don't really buy the "making it hard" part. If people aren't looking at their tests, and reviewers are looking at the tests either, then I don't think we can help them.

It also seems to me that minimal CHECK lines come from minimal input. I'd rather have consistent tests (i.e. automated with good formatting) that are easy to read and update, but perhaps more verbose, than CHECK lines which are absolutely minimal, but are poorly documented about what they are testing, or missing so much context that they are impossible to debug when they fail. Can you suggest some good 'heavily minimized' tests, so we can see how much difference there is between what is automatically generated and what is human-minimal? I'd like to encourage thought be put into tests, but temper it with some practicality.

Yes, the script throws away user edits. I think to not do that we need something significantly smarter than a bunch of REGEXes (along the lines of an MLIR dialect for CHECK lines.)

[jdoerfert]

In D83924#2157317, @stephenneuendorffer wrote:

In D83924#2156968, @jdoerfert wrote:

[Drive by] tests?

Yeah, would be good idea.

Sorry, I should have used more words. There is a way to add lit tests for these scripts now, see llvm/test/tools/UpdateTestChecks. :)

[mehdi_amini]

llvm/utils/update_mlir_test_checks.py

29

It also seems to me that minimal CHECK lines come from minimal input.

I think that is only half of it. Minimizing the input isn't enough to minimize the CHECK.
What would the script generate here for instance: https://github.com/llvm/llvm-project/blob/master/mlir/test/Transforms/sccp.mlir ?

[silvas]

llvm/utils/update_mlir_test_checks.py

29

There's a couple reasons why we might want to build in a bit of hand-holding into the script:

1. Not all MLIR development happens in MLIR core upstream and is subjected to the same level of review
2. Not all MLIR development is being done by people with extensive experience with FileCheck tests/best practices

I definitely sympathize with the idea that minimized check lines and result in difficult to update/understand tests (and tests that require a lot more staring-at to absorb their documentation value). While it is easier for test maintainers to update tests if they just need to run the script, it also loses a lot of value of the test (what if the test is now actually "failing"?). Except for crashes, simply updating the checks blindly with a script will let bugs slip through.

I think the idea is that a properly minimized test (both the IR and the set of things it is checking) won't be broken by practically any changes other than ones that truly break it (or are merely cosmetic, like op syntax changes) and need manual attention anyway.

llvm/utils/UpdateTestChecks/common.py

def __init__(self, test, parser, script_name, input_lines, args, argv,
     self.parser = parser
     self.path = test
     self.args = args
     self.argv = argv
     self.input_lines = input_lines
     self.run_lines = find_run_lines(test, self.input_lines)
     self.comment_prefix = comment_prefix
     if self.comment_prefix is None:
-      if self.path.endswith('.mir'):
+      if self.path.endswith('.mlir'):
+        self.comment_prefix = '//'
+      elif self.path.endswith('.mir'):
         self.comment_prefix = '#'
       else:
         self.comment_prefix = ';'
     self.autogenerated_note_prefix = self.comment_prefix + ' ' + UTC_ADVERT
     self.test_autogenerated_note = self.autogenerated_note_prefix + script_name
     self.test_autogenerated_note += get_autogennote_suffix(parser, self.args)
 
   def iterlines(self, output_lines):

llvm/utils/update_mlir_test_checks.py

+#!/usr/bin/env python3
+
+"""A script to generate FileCheck statements for 'mlir_opt' regression tests.
+
+This script is a utility to update MLIR opt test cases with new
+FileCheck patterns. It can either update all of the tests in the file or
+a single test function.
+
+Example usage:
+$ update_mlir_test_checks.py --opt=../bin/opt test/foo.mlir
+
+Workflow:
+1. Make a compiler patch that requires updating some number of FileCheck lines
+   in regression test files.
+2. Save the patch and revert it from your local work area.
+3. Update the RUN-lines in the affected regression tests to look canonical.
+   Example: "; RUN: opt < %s -instcombine -S | FileCheck %s"
+4. Refresh the FileCheck lines for either the entire file or select functions by
+   running this script.
+5. Commit the fresh baseline of checks.
+6. Apply your patch from step 1 and rebuild your local binaries.
+7. Re-run this script on affected regression tests.
+8. Check the diffs to ensure the script has done something reasonable.
+9. Submit a patch including the regression test diffs for review.
+
+A common pattern is to have the script insert complete checking of every
+instruction. Then, edit it down to only check the relevant instructions.
+The script is designed to make adding checks to a test case fast, it is *not*
+designed to be authoratitive about what constitutes a good test!
+"""
+
+from __future__ import print_function
+
+import argparse
+import glob
+import itertools
+import os  # Used to advertise this file's name ("autogenerated_note").
+import string
+import subprocess
+import sys
+import tempfile
+import re
+
+from UpdateTestChecks import common
+
+
+# Regex command to match an SSA identifier.
+SSA_RE_STR = '[0-9]+|[a-zA-Z$._-][a-zA-Z0-9$._-]*'
+SSA_RE = re.compile(SSA_RE_STR)
+
+MLIROPT_FUNCTION_RE = re.compile(
+    r'^\s*func\s+[^@]*@(?P<func>[\w.-]+?)\s*'
+    r'(?P<args_and_sig>\((\)|(.*?[\w.-]+?)\))[^{]*)\{\n(?P<body>.*?)^\}$',
+    flags=(re.M | re.S))
+
+MLIR_FUNCTION_RE = re.compile(r'^\s*func\s+[^@]*@([\w.-]+)\s*\(')
+
+# Class used to generate and manage string substitution blocks for SSA value
+# names.
+class SSAVariableNamer:
+
+  def __init__(self):
+    self.scopes = []
+    self.name_counter = 0
+
+  # Generate a substitution name for the given ssa value name.
+  def generate_name(self, ssa_name):
+    variable = 'VAL_' + str(self.name_counter)
+    self.name_counter += 1
+    self.scopes[-1][ssa_name] = variable
+    return variable
+
+  # Push a new variable name scope.
+  def push_name_scope(self):
+    self.scopes.append({})
+
+  # Pop the last variable name scope.
+  def pop_name_scope(self):
+    self.scopes.pop()
+
+
+# Process a line of input that has been split at each SSA identifier '%'.
+def process_line(line_chunks, variable_namer):
+  output_line = ''
+
+    # Process the rest that contained an SSA value name.
+  for chunk in line_chunks:
+    m = SSA_RE.match(chunk)
+    ssa_name = m.group(0)
+
+    # Check if an existing variable exists for this name.
+    variable = None
+    for scope in variable_namer.scopes:
+      variable = scope.get(ssa_name)
+      if variable is not None:
+        break
+
+    # If one exists, then output the existing name.
+    if variable is not None:
+      output_line += '%[[' + variable + ']]'
+    else:
+      # Otherwise, generate a new variable.
+      variable = variable_namer.generate_name(ssa_name)
+      output_line += '%[[' + variable + ':.*]]'
+
+    # Append the non named group.
+    output_line += chunk[len(ssa_name):]
+
+  return output_line
+
+
+# Pre-process a line of input to remove any character sequences that will be
+# problematic with FileCheck.
+def preprocess_line(line):
+  # Replace any double brackets, '[[' with escaped replacements. '[['
+  # corresponds to variable names in FileCheck.
+  output_line = line.replace('[[', '{{\\[\\[}}')
+
+  # Replace any single brackets that are followed by an SSA identifier, the
+  # identifier will be replace by a variable; Creating the same situation as
+  # above.
+  output_line = output_line.replace('[%', '{{\\[}}%')
+
+  return output_line
+
+# Hacked from generate-test-checks.py.  Not sure how to do this with
+# common.add_ir_checks
+def add_mlir_checks(output_lines, args, raw_tool_output, variable_namer):
+  # A map containing data used for naming SSA value names.
+  variable_namer = SSAVariableNamer()
+  input_lines = raw_tool_output.split('\n')
+  for input_line in input_lines:
+    lstripped_input_line = input_line.lstrip()
+
+    if not lstripped_input_line:
+      continue
+
+    # Lines with blocks begin with a ^. These lines have a trailing comment
+    # that needs to be stripped.
+    is_block = lstripped_input_line[0] == '^'
+    if is_block:
+      input_line = input_line.rsplit('//', 1)[0].rstrip()
+
+    # Top-level operations are heuristically the operations at nesting level 1.
+    is_toplevel_op = (not is_block and input_line.startswith('  ') and
+                      input_line[2] != ' ' and input_line[2] != '}')
+
+    # If the line starts with a '}', pop the last name scope.
+    if lstripped_input_line[0] == '}':
+      variable_namer.pop_name_scope()
+
+    # If the line ends with a '{', push a new name scope.
+    if input_line[-1] == '{':
+      variable_namer.push_name_scope()
+
+    # Preprocess the input to remove any sequences that may be problematic with
+    # FileCheck.
+    input_line = preprocess_line(input_line)
+
+    # Split the line at the each SSA value name.
+    ssa_split = input_line.split('%')
+
+    # If this is a top-level operation use 'CHECK-LABEL', otherwise 'CHECK:'.
+    if not is_toplevel_op or not ssa_split[0]:
+      output_line = '// ' + args.check_prefix + ': '
+      # Pad to align with the 'LABEL' statements.
+      output_line += (' ' * len('-LABEL'))
+
+      # Output the first line chunk that does not contain an SSA name.
+      output_line += ssa_split[0]
+
+      # Process the rest of the input line.
+      output_line += process_line(ssa_split[1:], variable_namer)
+
+    else:
+      # Append a newline to the output to separate the logical blocks.
+      output_lines.append('')
+      output_line = '// ' + args.check_prefix + '-LABEL: '
+
+      # Output the first line chunk that does not contain an SSA name for the
+      # label.
+      output_line += ssa_split[0] + '\n'
+
+      # Process the rest of the input line on a separate check line.
+      if len(ssa_split) > 1:
+        output_line += '// ' + args.check_prefix + '-SAME:  '
+
+        # Pad to align with the original position in the line.
+        output_line += ' ' * len(ssa_split[0])
+
+        # Process the rest of the line.
+        output_line += process_line(ssa_split[1:], variable_namer)
+    # If it's outside a function, it just gets copied to the output.
+    output_lines.append(output_line)
+
+def main():
+  from argparse import RawTextHelpFormatter
+  parser = argparse.ArgumentParser(description=__doc__, formatter_class=RawTextHelpFormatter)
+  parser.add_argument('--opt-binary', default='mlir-opt',
+                      help='The opt binary used to generate the test case')
+  parser.add_argument(
+      '--function', help='The function in the test file to update')
+  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(
+      '--check-prefix', default='CHECK', help='Prefix to use from check file.')
+  parser.add_argument('tests', nargs='+')
+  initial_args = common.parse_commandline_args(parser)
+
+  script_name = os.path.basename(__file__)
+  opt_basename = os.path.basename(initial_args.opt_binary)
+  if not re.match(r'^(.)*opt(-\d+)?$', opt_basename):
+    common.error('Unexpected opt name: ' + opt_basename)
+    sys.exit(1)
+
+  for ti in common.itertests(initial_args.tests, parser,
+                             script_name='utils/' + script_name):
+    # If requested we scrub trailing attribute annotations, e.g., '#0', together with whitespaces
+    if ti.args.scrub_attributes:
+      common.SCRUB_TRAILING_WHITESPACE_TEST_RE = common.SCRUB_TRAILING_WHITESPACE_AND_ATTRIBUTES_RE
+    else:
+      common.SCRUB_TRAILING_WHITESPACE_TEST_RE = common.SCRUB_TRAILING_WHITESPACE_RE
+
+    prefix_list = []
+    for l in ti.run_lines:
+      if '|' not in l:
+        common.warn('Skipping unparseable RUN line: ' + l)
+        continue
+
+      (tool_cmd, filecheck_cmd) = tuple([cmd.strip() for cmd in l.split('|', 1)])
+      common.verify_filecheck_prefixes(filecheck_cmd)
+      if not tool_cmd.startswith(opt_basename + ' '):
+        common.warn('Skipping non-%s RUN line: %s' % (opt_basename, l))
+        continue
+
+      if not filecheck_cmd.startswith('FileCheck '):
+        common.warn('Skipping non-FileChecked RUN line: ' + l)
+        continue
+
+      tool_cmd_args = tool_cmd[len(opt_basename):].strip()
+      tool_cmd_args = tool_cmd_args.replace('< %s', '').replace('%s', '').strip()
+
+      check_prefixes = [item for m in
+                        common.CHECK_PREFIX_RE.finditer(filecheck_cmd)
+                        for item in m.group(1).split(',')]
+      if not check_prefixes:
+        check_prefixes = ['CHECK']
+
+      # FIXME: We should use multiple check prefixes to common check lines. For
+      # now, we just ignore all but the last.
+      prefix_list.append((check_prefixes, tool_cmd_args))
+
+    func_dict = {}
+    for prefixes, _ in prefix_list:
+      for prefix in prefixes:
+        func_dict.update({prefix: dict()})
+    for prefixes, opt_args 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)
+      common.build_function_body_dictionary(
+              MLIROPT_FUNCTION_RE, common.scrub_body, [],
+              raw_tool_output, prefixes, func_dict, ti.args.verbose,
+              ti.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])
+    common.debug('Rewriting FileCheck prefixes:', str(prefix_set))
+    output_lines = []
+
+    # A map containing data used for naming SSA value names.
+    variable_namer = SSAVariableNamer()
+    for input_line_info in ti.iterlines(output_lines):
+      input_line = input_line_info.line
+      args = input_line_info.args
+      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
+
+        # # Print out the various check lines here.
+        # common.add_ir_checks(output_lines, '//', prefix_list, func_dict,
+        #                    func_name, args.preserve_names, args.function_signature)
+        add_mlir_checks(output_lines, args, raw_tool_output, variable_namer)
+        
+        is_in_function_start = False
+
+      if is_in_function:
+        if common.should_add_line_to_output(input_line, prefix_set):
+          # This input line of the function body will go as-is into the output.
+          # Except make leading whitespace uniform: 2 spaces.
+#          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
+
+      # If it's outside a function, it just gets copied to the output.
+      output_lines.append(input_line)
+
+      m = MLIR_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:
+        # When filtering on a specific function, skip all others.
+        continue
+      is_in_function = is_in_function_start = True
+
+    common.debug('Writing %d lines to %s...' % (len(output_lines), ti.path))
+
+    with open(ti.path, 'wb') as f:
+      f.writelines(['{}\n'.format(l).encode('utf-8') for l in output_lines])
+
+
+if __name__ == '__main__':
+  main()