[llvm] a33b40d - [NFC][DebugInfo] Autogenerate check lines in assignment-tracking/sroa/*

[Roman Lebedev via llvm-commits]

On Thu, Dec 15, 2022 at 10:37 PM Robinson, Paul <paul.robinson at sony.com> wrote:
>
> > > I am concerned that blindly converting to scripted checks
> > > is going to lose more coverage than the one example that I
> > > happened to notice by chance here.
> > I have no interest in converting every (debuginfo or not) test
> > to use the script. I simply do not wish to entertain the idea that
> > one has to spend many tens of minutes manually updating check lines
> > in tests where there are tools for that particular area of tests.
> > (Aka, i'm doing this because upcoming patch touches these tests)
> >
> > > What validation do we
> > > have that the script is doing reasonable and correct things?
> > Define reasonable and correct?
>
> Doesn't throw away tests of debug-info metadata that the test
> had originally, for one thing.
I'm afraid this is a non-starter by intentional design.
The first thing it does is cleans the slate,
removes existing check lines :)

> > Pretty much all LLVM tests use it.
>
> In the areas you work on, possibly; but certainly not LLVM overall.
> check-llvm reports 45,517 tests; I get 14,577 hits on the string
> "Assertions have been autogenerated."  Let's be generous and
> call it one-third. Not "pretty much all."
>
> I have to say, it's relatively rarely that I encounter a test
> with automated checks.  I suppose I have ever used the scripts,
> but unlikely more than once or twice a year.  The areas you
> work on and the areas I work on clearly don't overlap much, but
> that doesn't mean your tools are correct and valid everywhere;
> clearly they are not used in MOST of LLVM, and they are DEFINITELY
> not prepared to handle debug-info tests.

Ok, i should have been more specific :)
In the areas i have touched, pretty much every single test
uses them and all new tests (in last 5+ years or so?)
are strongly recommended to use them. I'm sure of that.

```
llvm/test$ find -iname *.ll | wc -l
31231
llvm/test$ grep -r "Assertions have been autogenerated" | wc -l
14578
llvm/test$ cd Transforms/
llvm/test/Transforms$ find -iname *.ll | wc -l
7919
llvm/test/Transforms$ grep -r "Assertions have been autogenerated" | wc -l
3903
llvm/test/CodeGen$ find -iname *.ll | wc -l
17849
llvm/test/CodeGen$ grep -r "Assertions have been autogenerated" | wc -l
8988
```
So it is nowhere near "Let's be generous and call it one-third",
but "almost exactly half" of all existing `.ll` tests.
Obviously, there are non-.ll tests, and obviously
they are going to skew the stats in favor of your argument :)

> > > I see that you modified the script; did anyone with
> > > debug-info expertise review that change?
> > I only told it that the metadata that is used by !DIAssignID is a
> > metadata,
> > that should not be hardcoded in check-output but named and checked.
> >
> > Also, !DIAssignID is not documented in LangRef, it seems?
>
> And there are many other debug-info metadata tags, which apparently
> the script is not aware of?
That can be improved. Which ones? Are they also missing from LangRef?

> The next time one of your patches
> affects a debug-info test, how will anyone know that coverage has
> been lost?  We would never have known for this patch. How much test
> coverage are you willing to throw away?
I think we are missing the main point here. Let's take a step back.

If i was interested in throwing away tests, i would have simply unintentionally
manually regressed the test coverage when trying to manually update them,
and you would indeed be none the wiser. Should have i done just that?

Instead, i'm trying to do the opposite, and avoid the possibility of that,
by having a single source of unbiased truth.

And please, i hope, by "your patch" you mean "any patch by anyone for which
there happens to be DebugInfo test coverage"?

> > > Please do not XFAIL any debug-info tests without filing
> > > an issue first, and if you could cite the issue in a comment
> > > by the XFAIL, that would be very helpful.
> > Bingo! And that's precisely the problem.
> >
> > If you can not rely on a script to do the update,
> > can you *really* rely on the people, that have absolutely no idea
> > how the debug info is represented in LLVM IR,
> > to *manually* update these tests,
> > more correctly than the script would?
> >
> > Or should every single patch that would require
> > updates to debuginfo tests be required
> > to go through someone from DebugInfo team?
>
> Until we have confidence that the scripts aren't throwing away
> useful checks, like they did this time, maybe they should.
>
> --paulr