[UTC] Add fallback support for specific metadata, and check their defs

[hnrklssn]

Summary

This prevents update_cc_tests.py from emitting hard-coded identifiers
for metadata (global variable checkers still check hard-coded
identifiers). Instead it emits regex checkers that match even if the
identifiers change. Also adds a new mode for --check-globals: instead of
simply being on or off, it now has the options 'none', 'smart' and
'all', with 'none' and 'all' corresponding to the previous modes.

The 'smart' mode only emits checks for global definitions referenced
in the IR or other metadata that itself has a definition checker
emitted, making the rule transitive. It does not emit checks for
attribute sets, since that is better checked by --check-attributes. This
mode is made the new default. To make the change in default mode
backwards compatible a version bump is introduced (to v3), and the
default remains 'none' in v1 & v2.

This will result in metadata checks being emitted more often, so filters
are added to not check absolute file paths and compiler version git
hashes.

rdar://105239218

[hnrklssn]

I'm not familiar with any upstream ways to trigger !annotations so I unfortunately don't have any test updates to upstream at this time. I'm open to suggestions for adding ways to test the change though.

[arichardson]

Change looks good to me, but if clang can't emit these yet we should probably wait until committing this? Do you have a link to a review that uses this metadata?

[nikic]

We should probably just use the META fallback for all metadata not already explicitly handled. I think there's pretty little value in adding a special name for each metadata kind.

[hnrklssn]

Add test case emitting !annotation from clang.
Make annotation matching a generic metadata fallback.

[hnrklssn]

In D148216#4266174, @arichardson wrote:

Change looks good to me, but if clang can't emit these yet we should probably wait until committing this? Do you have a link to a review that uses this metadata?

It can emit them, we just use them a bit more frequently downstream. I've found an example and added a lit test as an example of how it works.

[hnrklssn]

In D148216#4266201, @nikic wrote:

We should probably just use the META fallback for all metadata not already explicitly handled. I think there's pretty little value in adding a special name for each metadata kind.

I added a fallback, and some logic to extract the metadata kind name from the IR, because I do find it useful to emit checks named after the type of IR they match against.

[delcypher]

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

12

@hnrklssn I just noticed we don't have a CHECK for what META2 actually refers to. Should we?

Not something that has to be fixed in this patch, more just an observation.

llvm/utils/UpdateTestChecks/common.py

703

Is this call to re.search(...) guaranteed to always succeed?

779

There may be some value in having !annotations matched explicitly as well as there being a fallback. In the current patch it looks like:

• metadata is assigned variables with the META prefix on CHECK lines
• !annotation is assigned variables with the META prefix on CHECK lines
• Anything else that matches r'![a-z.]+ ' is assigned variables with the META prefix on CHECK lines

When looking a large test having everything lumped into META variables could make the generated tests a little harder to read.

[hnrklssn]

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

12

Indeed this is true for metadata in general, presumably because the RHS often refer to things like other metadata identifiers. In the case of annotations they seem to always refer to simple strings however, so it would be feasible to do a straight match without having to do recursive matching or complex regexes to determine which part of the metadata to match against.

In many cases with metadata attached to IR nodes, multiple nodes refer to the same metadata node, so at least you verify that they still are consistent. But I agree that verifying the content would be a great future addition.

llvm/utils/UpdateTestChecks/common.py

703

Yes. match is a match object from a combined regex concatenated from both the IR prefix (![a-z.]+ ) and the IR regexp (![0-9]+), so it will always contain a substring matching the IR prefix.

779

In a way, yes. At the same time, since we're not matching against the definition of the metadata node, all uses (while named META#) will still be prefixed by !annotation. On the other hand again, we have explicitly enumerated so many other types of metadata, so it is a bit inconsistent.

[nikic]

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

12

You need to pass --check-globals to check the actual metadata.

[hnrklssn]

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

12

When I add that to this test case it adds

//.
// CHECK: attributes #0 = { noinline nounwind optnone "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
//.
// CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
// CHECK: !1 = !{!"clang version 17.0.0 ([e-mail]:llvm/llvm-project.git 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
// CHECK: !2 = !{!"auto-init"}
//.

So it seems to just be doing a simple literal matching on all metadata, regardless of whether we captured that metadata in any filecheck variable. And it still doesn't use the META2 variable to match the definition. Am I missing something? If we use the literal metadata names instead of variable matching for the definitions, there isn't much point in doing variable matching for the metadata uses either, since the test still very much relies on the metadata numbering being stable.

[hnrklssn]

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

12

@nikic Do you have more information to add about how metadata definition matchers can be generated without hardcoding everything (which is kind of the opposite of what this patch is trying to do), or in general if you're happy with the state of the PR?

[nikic]

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

12

This works fine with update_test_checks, so it must be some bug in update_cc_test_checks in particular. From a quick look, I suspect it's because https://github.com/llvm/llvm-project/blob/3d05ab6d3e24e76ff53b8d7d623c436b4be5b809/llvm/utils/update_cc_test_checks.py#L447 hardcodes a True value, while update_test_checks makes this dependent on --preserve-names, which is disabled by default there.

[nikic]

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

12

Or maybe just test this with update_test_checks? This change is valuable for that script as well, and it doesn't have this extra issue.

[hnrklssn]

Overhauled how update_cc_tests.py checks globals

It now:

• no longer matches the literal names, like update_test_checks.py does unless --preserve-names (which doesn't seem to see much use, at least not upstream, so did not add option to update_cc_tests.py) is set
• (along with update_test_checks.py) now adds checks for any definitions of globals referenced in the IR (by default, can still check all globals using "--check-globals all")
• (along with update_test_checks.py) can also be configured to emit checks for any definitions of globals referenced by other globals referenced in the IR, using "--check-globals transitive"
• (along with update_test_checks.py) filters out unstable metadata output, so that literal directory names and git hashes are not matched against

[hnrklssn]

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

12

I couldn't find any uses of
--preserve-names in the entire repo (not even in tests for update_test_checks), so I went ahead and changed update_cc_test_checks to pass True instead.

While at it I added 2 new options to match some, but not necessarily all, globals. I set the least verbose one (other than "none") as the default, emitting checks for the definitions of all globals that we emit matches for in the IR. Do you agree that this is reasonable?

[nikic]

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

12

That sounds like a nice approach. My main question would be whether --check-globals seen is the right default: My gut feeling here is that this will introduce additional checks for attributes/metadata in tests that don't actually care about it, but I'm not particularly sure about that.

You might want to do some mass test regeneration and take a look over the diff to see how useful the changes would be.

If we do change the default, the default change likely need to be part of --version 3 to avoid substantial test churn.

[hnrklssn]

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

12

I've regenerated checks before and after my patch and compared them. Some findings:

• there's a surprising number of custom metadata types that didn't get variable checkers but instead checked hardcoded metadata IDs (!sloc, !pcsections, !alias.scope, !noalias, !nonnull, !noundef, !dereferenceable, !nontemporal, !fpmath etc.). The tests often have checkers, but were removed when regenerating them before my patch, so something seems to have broken.
• I'm split on whether global variables should match the exact name or get a matcher. Right now the patch handles them like any other global, but I'm assuming that the name actually carries significance for linking?
• matching attributes seems useful in some cases, not matter too much in others, but often there aren't that many attributes so it does not pollute much. Tests where attribute matchers pollute should probably use --scrub-attributes in update_check_tests (does not exist in update_cc_check_tests) anyways
• tests that include debug info generally do so deliberately, so matching against that seems reasonable
• TBAA metadata doesn't give too much insight unless transitive/all. That said not all TBAA tests check metadata at all today (at least not llvm/test/Transforms/ArgumentPromotion/reserve-tbaa.ll)
• Similar goes for !llvm.loop, the useful info is pointed to indirectly
• Profile info doesn't seem to be included very often. It's probably reasonable to match against it when it's there. Found one test that was specifically created to check profile info metadata but lost the check during conversion to update_check_tests (llvm/test/Transforms/ArgumentPromotion/profile.ll)

Since the transitive setting provides substantially more insight than the seen setting for loop and tbaa, and only really explodes for debug info (which is rarely included, and deliberately so when done) I'm open to making it the default instead. For opt based tests I kind of think that all should be the default since any metadata in the output was either created by the tested passes, or explicitly included in the input.

[nikic]

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

12

I've regenerated checks before and after my patch and compared them. Some findings:

• there's a surprising number of custom metadata types that didn't get variable checkers but instead checked hardcoded metadata IDs (!sloc, !pcsections, !alias.scope, !noalias, !nonnull, !noundef, !dereferenceable, !nontemporal, !fpmath etc.). The tests often have checkers, but were removed when regenerating them before my patch, so something seems to have broken.

Yes, which is why I'm happy to have the generic implementation here -- many of these were handled by manually adjusting the update_test_checks output...

• I'm split on whether global variables should match the exact name or get a matcher. Right now the patch handles them like any other global, but I'm assuming that the name actually carries significance for linking?

I'm thinking we should probably retain the name of globals, the same way we do for functions. Unlike things like local variable names, and attribute/metadata IDs, these tend to be stable and semantically meaningful. (Also the wildcard generated for them breaks syntax highlighting in vim. Grumble, grumble.)

• matching attributes seems useful in some cases, not matter too much in others, but often there aren't that many attributes so it does not pollute much. Tests where attribute matchers pollute should probably use --scrub-attributes in update_check_tests (does not exist in update_cc_check_tests) anyways
• tests that include debug info generally do so deliberately, so matching against that seems reasonable
• TBAA metadata doesn't give too much insight unless transitive/all. That said not all TBAA tests check metadata at all today (at least not llvm/test/Transforms/ArgumentPromotion/reserve-tbaa.ll)
• Similar goes for !llvm.loop, the useful info is pointed to indirectly
• Profile info doesn't seem to be included very often. It's probably reasonable to match against it when it's there. Found one test that was specifically created to check profile info metadata but lost the check during conversion to update_check_tests (llvm/test/Transforms/ArgumentPromotion/profile.ll)

Since the transitive setting provides substantially more insight than the seen setting for loop and tbaa, and only really explodes for debug info (which is rarely included, and deliberately so when done) I'm open to making it the default instead. For opt based tests I kind of think that all should be the default since any metadata in the output was either created by the tested passes, or explicitly included in the input.

I think there's broadly two cases:

1. There are globals/attributes/metadata in the input, which are needed to make the test work, but not important for the test output, because they will just be preserved. E.g. an InstSimplify test might use globals/attributes/metadata, but won't ever change them, and checking that fact is not particularly useful.

2. Tests for passes that do modify/add attributes/metadata, such as IPO inference passes. It's worth noting that there is a separate --check-attributes flag that covers some of these cases, which will match the attributes printed before the function definition, which is generally more convenient than matching the attributes at the end of the module.

The first the use case we generally do not want to match any globals, and for the latter we probably want either --check-attributes (if testing function attributes in particular) or --check-globals transitive (if testing something metadata or call-site attributes).

I'm still not really sure what the right default would be. I guess one could argue that for the former case, having a few extra attributes at the test isn't a big deal -- one can just ignore them. But that does also mean that whenever we for example change attributes on an intrinsic, this will now suddenly affect hundreds of tests that encode those attributes.

[hnrklssn]

Revert checker behaviour for global variables to check hard-coded identifiers
Remove 'seen' mode
Replace 'transitive' mode with 'smart', where attributes are no longer checked as metadata
Introduce version bump to v3, and make 'smart' mode the default in v3

[hnrklssn]

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

12

I replaced --checked-globals transitive with --checked-globals smart, with the addition that it doesn't check attributes, and made it the default mode in the new version 3. If attributes need to be checked --check-attributes can be used, or --check-globals all.

Emitting checks for metadata that hasn't been changed by the passes when testing with opt may result in some extra noise in the test, but is unlikely to result in significant extra work to maintain, since if the metadata format for that metadata kind changes, the metadata input needs to change also. For test cases where it adds significant noise --check-globals none is always an option.

Also reverted the behaviour for global variables to check the exact identifier again.

[hnrklssn]

@nikic Did you have a look at the new patch?

[hnrklssn]

@nikic Ping :)

[nikic]

Could you please rebase the patch to current main? I wasn't able to apply it.

The behavior you describe sounds reasonable to me.

llvm/utils/UpdateTestChecks/common.py

1215

We should also not print the all argument for --check-globals argument for version < 3, otherwise that will introduce a spurious change in all such tests.

[jdoerfert]

could you put a little more information in the commit message please. "It won't do X when we do Y", could mean a lot of things. We don't do Y anymore, or we do X' now, with various choices for X'.

[hnrklssn]

Rebase after reformatting with Black, update commit message

[hnrklssn]

In D148216#4431456, @jdoerfert wrote:

could you put a little more information in the commit message please. "It won't do X when we do Y", could mean a lot of things. We don't do Y anymore, or we do X' now, with various choices for X'.

I've updated the commit message to clarify what it means to not emit hardcoded identifiers, and that we instead emit regex checkers. Was that what you were aiming for, or are there other parts that you would like me to clarify?

llvm/utils/UpdateTestChecks/common.py

1215

I don't know how to dynamically change the --check-globals option between store_true and choices, since the behaviour can itself be affected by which argument is passed to the --version option. So the way I see it there's no way of knowing how to parse between two different option types ahead of time. For the default when no option is specified the parsing is the same however, so we can simply infer later what option is implied based on the version.

[nikic]

llvm/utils/UpdateTestChecks/common.py

1215

It's not necessary to change the option, just how it is printed. I.e. add something like this:

if args.version < 3 and value == "all":
    # Don't include argument value in older versions.
    autogenerated_note_args += "--check-globals "
    continue

[nikic]

llvm/utils/UpdateTestChecks/common.py

1215

Ah, I get what you're saying now -- old UTC_ARGS are not accepted at all anymore, and trying to update existing tests will just fail with an error!

We do need old UTC_ARGS to work. I think you can make the argument value optional using these options:

nargs="?", 
const="default",
default="default",
choices=["none", "smart", "all", "default"],

Or possibly omit const/default and handle None instead.

[nikic]

llvm/utils/UpdateTestChecks/common.py

1215

Correction, I think the right options are:

nargs="?",
const="all",
default="default",
choices=["none", "smart", "all"],

That is, use "default" is it's not specified at all, use "all" if only --check-globals is passed and also accept --check-globals none|smart|all.

[hnrklssn]

Keep supporting --check-globals without explicit level as meaning '--check-globals all'

[hnrklssn]

llvm/utils/UpdateTestChecks/common.py

1215

Ah nice, I did not expect the library to support this use case!

[nikic]

There are some test failures.

I believe there is one bug with the handling of unnamed globals. Previously we produced this:

; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals --version 2
; RUN: opt -S < %s | FileCheck %s
@0 = global i32 0

;.
; CHECK: @[[GLOB0:[0-9]+]] = global i32 0
;.
define i32 @test() {
; CHECK-LABEL: define i32 @test() {
; CHECK-NEXT:    [[V:%.*]] = load i32, ptr @[[GLOB0]], align 4
; CHECK-NEXT:    ret i32 [[V]]
;
  %v = load i32, ptr @0
  ret i32 %v
}

And now we instead check CHECK: @0 = global i32 0, so there is a mismatch between definition and use. I believe it does make sense to keep the wildcard names for unnamed globals.

On the same test case, if I keep rerunning update_test_checks on the same file, it keeps adding extra CHECK: @0 = global i32 0 lines at the start (before the first ;.. That didn't happen previously.

clang/test/utils/update_cc_test_checks/Inputs/annotations.c

2

It would be more typical to pass --version 3 when calling update_cc_test_checks. (Note that this is only a quirk of the test setup. Outside tests the --version 3 is implied for new tests.)

llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_attrs.ll.funcattrs.expected

9 ↗

(On Diff #532719)

Huh, where does this change come from?

llvm/utils/UpdateTestChecks/common.py

780

Does this still comply with the formatter?

[hnrklssn]

Added back regex matchers for anonymous global values

[hnrklssn]

In D148216#4434171, @nikic wrote:

There are some test failures.

I believe there is one bug with the handling of unnamed globals. Previously we produced this:

; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals --version 2
; RUN: opt -S < %s | FileCheck %s
@0 = global i32 0

;.
; CHECK: @[[GLOB0:[0-9]+]] = global i32 0
;.
define i32 @test() {
; CHECK-LABEL: define i32 @test() {
; CHECK-NEXT:    [[V:%.*]] = load i32, ptr @[[GLOB0]], align 4
; CHECK-NEXT:    ret i32 [[V]]
;
  %v = load i32, ptr @0
  ret i32 %v
}

And now we instead check CHECK: @0 = global i32 0, so there is a mismatch between definition and use. I believe it does make sense to keep the wildcard names for unnamed globals.

On the same test case, if I keep rerunning update_test_checks on the same file, it keeps adding extra CHECK: @0 = global i32 0 lines at the start (before the first ;.. That didn't happen previously.

Fixed. Does this test case exist somewhere? I couldn't find it upstream.

llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_attrs.ll.funcattrs.expected

9 ↗

(On Diff #532719)

I thought it was an existing test failure upstream, but it seems to have disappeared after doing a full rebuild clang+llvm.

llvm/utils/UpdateTestChecks/common.py

780

No, I guess maintaining this structure isn't worth it long term with the autoformatter

[hnrklssn]

@nikic Ping :)

[hnrklssn]

@nikic Are you happy with the current patchset?

[nikic]

LGTM

Fixed. Does this test case exist somewhere? I couldn't find it upstream.

That wasn't an existing test case. Though if you add a --check-globals all variant to generated-funcs.c, I believe that would cover that case.

[hnrklssn]

Add unnamed global test case

Added cases in generated-funcs.c with --check-globals all, testing that named and unnamed globals are matched differently.

[nikic]

(The patch description could use an update...)

[hnrklssn]

Update test cases after rebasing on ToT

[jdoerfert]

When you commit things, can you please add the Differential Revision into the commit message so it is closed here and we can easily find the review for a change.

Committed as 8a3fdf7b908978625e9a7e57fbb443e4e6f98976.

[jdoerfert]

This is broken: https://github.com/llvm/llvm-project/issues/63746

[jdoerfert]

I might pull this later today until a fix is available. It makes updates of files with globals super cumbersome (you need to manually remove the duplicates).

[jdoerfert]

In D148216#4486400, @jdoerfert wrote:

I might pull this later today until a fix is available. It makes updates of files with globals super cumbersome (you need to manually remove the duplicates).

FWIW, the duplication happens if you pass -p already. It's an existing problem made worse by effectively forcing -p for globals.

clang/test/utils/update_cc_test_checks/Inputs/annotations.c

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fblocks -ftrivial-auto-var-init=zero %s -emit-llvm -o - | FileCheck %s
+
+int foo() {
+    int x = x + 1;
+    return x;
+}

clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected

+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fblocks -ftrivial-auto-var-init=zero %s -emit-llvm -o - | FileCheck %s
+
+// CHECK-LABEL: @foo(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[X:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    store i32 0, ptr [[X]], align 4, !annotation [[META2:![0-9]+]]
+// CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:    [[ADD:%.*]] = add nsw i32 [[TMP0]], 1
+// CHECK-NEXT:    store i32 [[ADD]], ptr [[X]], align 4
+// CHECK-NEXT:    [[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:    ret i32 [[TMP1]]
+//
+int foo() {
+    int x = x + 1;
+    return x;
+}

clang/test/utils/update_cc_test_checks/annotations.test

+## Test that !annotation metadata is matched correctly
+
+# RUN: cp %S/Inputs/annotations.c %t.c && %update_cc_test_checks %t.c
+# RUN: diff -u %S/Inputs/annotations.c.expected %t.c

llvm/utils/UpdateTestChecks/common.py

class NamelessValue:
   def is_local_def_ir_value_match(self, match):
     return self.ir_prefix == '%'
 
   # Return true if this kind of IR value is "global", basically if it matches '#{{.*}}'.
   def is_global_scope_ir_value_match(self, match):
     return self.global_ir_rhs_regexp is not None
 
   # Return the IR prefix and check prefix we use for this kind or IR value,
-  # e.g., (%, TMP) for locals.
+  # e.g., (%, TMP) for locals. If the IR prefix is a regex, return the prefix
+  # used in the IR output
   def get_ir_prefix_from_ir_value_match(self, match):
-    return self.ir_prefix, self.check_prefix
+    return re.search(self.ir_prefix, match[0])[0], self.check_prefix
 
   # Return the IR regexp we use for this kind or IR value, e.g., [\w.-]+? for locals
   def get_ir_regex_from_ir_value_re_match(self, match):
     # for backwards compatibility we check locals with '.*'
     if self.is_local_def_ir_value_match(match):
       return '.*'
     return self.ir_regexp
 
ir_nameless_values = [
     NamelessValue(r'PROF'       , '!' , r'!prof '              , r'![0-9]+'             , None                 ) ,
     NamelessValue(r'TBAA'       , '!' , r'!tbaa '              , r'![0-9]+'             , None                 ) ,
     NamelessValue(r'TBAA_STRUCT', '!' , r'!tbaa.struct '       , r'![0-9]+'             , None                 ) ,
     NamelessValue(r'RNG'        , '!' , r'!range '             , r'![0-9]+'             , None                 ) ,
     NamelessValue(r'LOOP'       , '!' , r'!llvm.loop '         , r'![0-9]+'             , None                 ) ,
     NamelessValue(r'META'       , '!' , r'metadata '           , r'![0-9]+'             , None                 ) ,
     NamelessValue(r'META'       , '!' , r''                    , r'![0-9]+'             , r'(?:distinct |)!.*' ) ,
     NamelessValue(r'ACC_GRP'    , '!' , r'!llvm.access.group ' , r'![0-9]+'             , None                 ) ,
+    NamelessValue(r'META'       , '!' , r'![a-z.]+ '           , r'![0-9]+'             , None                 ) ,
 ]
 
 asm_nameless_values = [
     NamelessValue(r'MCINST'     , 'Inst#' , '<MCInst #'        , r'\d+'                 , r'.+', is_number=True, replace_number_with_counter=True),
     NamelessValue(r'MCREG'      , 'Reg:'  , '<MCOperand Reg:'  , r'\d+'                 , r'.+', is_number=True, replace_number_with_counter=True),
 ]
 
 def createOrRegexp(old, new):
   if not old:
if action.const is not None:  # action stores a constant (usually True/False)
         continue
     if parser.get_default(action.dest) == value:
       continue  # Don't add default values
     if action.dest == 'function_signature' and args.version >= 2:
       continue # Enabled by default in version 2
     if action.dest == 'filters':
       # Create a separate option for each filter element.  The value is a list
       # of Filter objects.
       for elem in value:
         opt_name = 'filter-out' if elem.is_filter_out else 'filter'
         opt_value = elem.pattern()
         new_arg = '--%s "%s" ' % (opt_name, opt_value.strip('"'))
         if new_arg not in autogenerated_note_args:
           autogenerated_note_args += new_arg
     else:
       autogenerated_note_args += action.option_strings[0] + ' '
       if action.const is None:  # action takes a parameter