[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added inline comments.


================
Comment at: clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:    [[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:    ret i32 [[TMP1]]
+//
----------------
delcypher wrote:
> @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.
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.


================
Comment at: llvm/utils/UpdateTestChecks/common.py:703
   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
 
----------------
delcypher wrote:
> Is this call to `re.search(...)` guaranteed to always succeed?
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.


================
Comment at: llvm/utils/UpdateTestChecks/common.py:779
     NamelessValue(r'ACC_GRP'    , '!' , r'!llvm.access.group ' , r'![0-9]+'             , None                 ) ,
+    NamelessValue(r'META'       , '!' , r'![a-z.]+ '           , r'![0-9]+'             , None                 ) ,
 ]
----------------
delcypher wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216