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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
