nikic 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]]
+//
----------------
hnrklssn wrote:
> nikic wrote:
> > nikic wrote:
> > > hnrklssn wrote:
> > > > hnrklssn wrote:
> > > > > nikic wrote:
> > > > > > hnrklssn wrote:
> > > > > > > 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.
> > > > > > You need to pass `--check-globals` to check the actual metadata.
> > > > > 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
> > > > > (g...@github.com: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.
> > > > @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?
> > > 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.
> > 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.
> 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?
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.
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
