[utils] change update_test_checks.py use of 'TMP' value names

[spatel]

Summary

As discussed in PR45951:
https://bugs.llvm.org/show_bug.cgi?id=45951

There's a potential name collision between -instnamer and/or manually-generated IR test files with update_test_checks.py because all of them try to use the variable name that should never be used: "tmp".

This patch proposes to reduce the odds of collision and adds a warning if we detect the problem. This will cause regression test churn when regenerating CHECK lines on existing files. We could do a mass update to get that out of the way all at once if that seems better.

[RKSimon]

do we have any idea of the likely scope of the test changes?

[spatel]

In D80584#2057936, @RKSimon wrote:

do we have any idea of the likely scope of the test changes?

Assuming I did this correctly - check for auto-generated files that contain 'TMP':

test $ grep -l TMP `grep -rl update_test_checks *` | wc -l
    1044

[RKSimon]

LGTM - let's go with this. I'd not bother with the mass update, but an example refresh as part of this patch commit would make sense.

[jdoerfert]

This needs a command line flag so people can change the prefix. The default for that flag should be "TMP" to minimize the noise in updates. We also should have a test.
FWIW, the UTC_FLAGS logic will "remember" the flag chosen by the user so if you need to change from "TMP" to something else it will be recorded and no further interaction will be necessary.

[spatel]

In D80584#2065560, @jdoerfert wrote:

This needs a command line flag so people can change the prefix. The default for that flag should be "TMP" to minimize the noise in updates. We also should have a test.
FWIW, the UTC_FLAGS logic will "remember" the flag chosen by the user so if you need to change from "TMP" to something else it will be recorded and no further interaction will be necessary.

I disagree with adding an option for this. We don't want to turn a bug into a feature and carry that complexity cost for future users forever. The anonymous variable name is an implementation detail of the script that newcomers should not have to think about.

But I understand that test churn causes grief for current users (I'm a big user of the script and take the blame for the bug).

The problem is that this script and -instnamer conflict perfectly on the name "tmp". One option that we didn't consider in the discussion in PR45951: we could leave this script using "TMP" if we change -instnamer to use something else. Does anyone see any legacy constraint on the "tmp" naming in -instnamer?

[spatel]

Effectively reverted (but still have the name conflict warning at least) with:
rGe5b877275673

If we can change instnamer, we can probably keep living with this script as-is. Alternatively, let's re-fix this script and mass update test files, so there's no general concern about churn of existing tests.

[jdoerfert]

First, I think the warning is strictly a good thing. Thanks for keeping that in!

I don't think the options are really "complexity to the user" given a sensible default (= the old value). If a user sees a warning that says: Name conflict, use --nameless-prefix=XXXXX, where XXXXX is a string not otherwise used in your IR, they will happily do so. The UTC_ARGS extension makes it permanent and no one needs to worry about this until they see a warning again. (If we can "hide" the option from the help message, I'm fine with that too.)

Adding a test is just something we should do as it is easy to accidentally break functionality like the warning.

[spatel]

In D80584#2066319, @jdoerfert wrote:

First, I think the warning is strictly a good thing. Thanks for keeping that in!

Sure - we've been missing that for a long time.

I don't think the options are really "complexity to the user" given a sensible default (= the old value). If a user sees a warning that says: Name conflict, use --nameless-prefix=XXXXX, where XXXXX is a string not otherwise used in your IR, they will happily do so. The UTC_ARGS extension makes it permanent and no one needs to worry about this until they see a warning again. (If we can "hide" the option from the help message, I'm fine with that too.)

I'm still not convinced that's worth doing. I did change instnamer though:
rGdd54432a0f5a
...so between the warning and at least 1 of these not using 'tmp', I think we're good enough.

Adding a test is just something we should do as it is easy to accidentally break functionality like the warning.

Yep, I didn't remember that we had tests for scripts. So we have that now, and there's a clang test that wiggles on this too...although that seems like we should generalize that file to avoid killing all of the bots like I did. :)

[jdoerfert]

I think we're good enough.

I agree. If this doesn't bother anyone anymore there is no need for any options ;)

Thanks for the test!

llvm/utils/UpdateTestChecks/common.py

 ##### Generator of LLVM IR CHECK lines
 
 SCRUB_IR_COMMENT_RE = re.compile(r'\s*;.*')
 
 # Match things that look at identifiers, but only if they are followed by
 # spaces, commas, paren, or end of the string
 IR_VALUE_RE = re.compile(r'(\s+)%([\w.-]+?)([,\s\(\)]|\Z)')
 
+NAMELESS_PREFIX = "NAMELESS"
+
 # Create a FileCheck variable name based on an IR name.
 def get_value_name(var):
   if var.isdigit():
-    var = 'TMP' + var
+    var = NAMELESS_PREFIX + var
   var = var.replace('.', '_')
   var = var.replace('-', '_')
   return var.upper()
 
 
 # Create a FileCheck variable from regex.
 def get_value_definition(var):
   return '[[' + get_value_name(var) + ':%.*]]'
 
 
 # Use a FileCheck variable.
 def get_value_use(var):
   return '[[' + get_value_name(var) + ']]'
 
 # Replace IR value defs and uses with FileCheck variables.
 def genericize_check_lines(lines, is_analyze, vars_seen):
   # This gets called for each match that occurs in
   # a line. We transform variables we haven't seen
   # into defs, and variables we have seen into uses.
   def transform_line_vars(match):
     var = match.group(2)
+    if NAMELESS_PREFIX.lower() in var.lower():
+      warn("Change IR value name '%s' to prevent possible conflict with scripted FileCheck name." % (var,))
     if var in vars_seen:
       rv = get_value_use(var)
     else:
       vars_seen.add(var)
       rv = get_value_definition(var)
     # re.sub replaces the entire regex match
     # with whatever you return, so we have
     # to make sure to hand it back everything