Allow update_test_checks.py to not scrub names

[greened]

Summary

Add a --preserve-names option to tell the script not to replace IR names.
Sometimes tests want those names. For example if a test is looking for a
modification to an existing instruction we'll want to keep the names to
identify the instruction.

[lebedev.ri]

I'm not sure this is a good idea.

[greened]

In D68081#1684028, @lebedev.ri wrote:

I'm not sure this is a good idea.

Can you explain a bit more? The use-case is testing a transformation that simply updates an instruction, such as adding metadata. The assumption is that the generated test will be edited for brevity (as the script comments indicate) and that the user knows that names could change. Many times such tests will never change names because they are trivial, only running a single pass that doesn't add or delete any Values.

[reames]

In D68081#1684028, @lebedev.ri wrote:

I'm not sure this is a good idea.

I also don't see the value in this. With autogenerated checks, the name of the instructions shouldn't be required to tell if the test is getting the correct behaviour and preserving the names makes test checks much more fragile.

[greened]

I also don't see the value in this. With autogenerated checks, the name of the instructions shouldn't be required to tell if the test is getting the correct behaviour and preserving the names makes test checks much more fragile.

So here's the situation I ran into. I have hundreds of tests to update. I don't want to match all of the IR, just selected instructions. The current behavior of rewriting the operands with FileCheck variable names makes it difficult to delete parts of the IR. Maintaining the original names makes it easy to delete IR.

Perhaps there is another way to handle situations like this. I am open to ideas!

[greened]

In D68081#1686308, @greened wrote:

Perhaps there is another way to handle situations like this. I am open to ideas!

One idea would be to have a mode that replaces names not with FileCheck variable references but with a simple regexp like `"%[^[:space:]]+". That's what I end up doing for the tests in question anyway. Does that seem better?

[reames]

In D68081#1686308, @greened wrote:

I also don't see the value in this. With autogenerated checks, the name of the instructions shouldn't be required to tell if the test is getting the correct behaviour and preserving the names makes test checks much more fragile.

So here's the situation I ran into. I have hundreds of tests to update. I don't want to match all of the IR, just selected instructions. The current behavior of rewriting the operands with FileCheck variable names makes it difficult to delete parts of the IR. Maintaining the original names makes it easy to delete IR.

This is not what an autogenerated test is. An auto-generated test includes the final state, and updating it is simply running the update script. We do not want to encourage customization of the output or manual workflows.

[greened]

In D68081#1686473, @reames wrote:

This is not what an autogenerated test is. An auto-generated test includes the final state, and updating it is simply running the update script. We do not want to encourage customization of the output or manual workflows.

Well, that's pretty much the opposite of what the script says:

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!

[reames]

In D68081#1686490, @greened wrote:

Well, that's pretty much the opposite of what the script says:

I stand corrected. Given that, the patch looks reasonable.

Not a use case I particular want to encourage, but I take your point about existing documented behaviour.

[greened]

For some reason the commit did not auto-update this. Closing with commit r373912/a14ffc7eb741de4fd7484350d11947dea40991fd.

[simon_tatham]

I had update_cc_test_checks.py give me a Python traceback just now which I think must be to do with this commit:

Traceback (most recent call last):
  File "llvm/utils/update_cc_test_checks.py", line 267, in <module>
    sys.exit(main())
  File "llvm/utils/update_cc_test_checks.py", line 255, in main
    common.add_ir_checks(output_lines, '//', run_list, func_dict, mangled)
TypeError: add_ir_checks() missing 1 required positional argument: 'preserve_names'

The new parameter to add_ir_checks is mandatory, but it's only been added to the call in update_test_checks.py, and not to the one in update_cc_test_checks.py.

llvm/utils/UpdateTestChecks/common.py

for checkprefix in checkprefixes:
               comment_marker, checkprefix, func_line))
         is_blank_line = False
 
       # Add space between different check prefixes and also before the first
       # line of code in the test function.
       output_lines.append(comment_marker)
       break
 
-def add_ir_checks(output_lines, comment_marker, prefix_list, func_dict, func_name):
+def add_ir_checks(output_lines, comment_marker, prefix_list, func_dict,
+                  func_name, preserve_names):
   # Label format is based on IR string.
   check_label_format = '{} %s-LABEL: @%s('.format(comment_marker)
-  add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name, check_label_format, False, False)
+  add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name,
+             check_label_format, False, preserve_names)
 
 def add_analyze_checks(output_lines, comment_marker, prefix_list, func_dict, func_name):
   check_label_format = '{} %s-LABEL: \'%s\''.format(comment_marker)
   add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name, check_label_format, False, True)
 
 
 def check_prefix(prefix):
   if not PREFIX_RE.match(prefix):

llvm/utils/update_test_checks.py

def main():
   parser.add_argument('-v', '--verbose', action='store_true',
                       help='Show verbose output')
   parser.add_argument('--opt-binary', default='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('-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('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):
for input_line in input_lines:
           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)
+        common.add_ir_checks(output_lines, ';', prefix_list, func_dict,
+                             func_name, args.preserve_names)
         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)