[PATCH] D116832: [UpdateLLCTestChecks] Allow replacing register names with variables

Wed Jan 12 10:48:36 PST 2022

greened added a comment.

In D116832#3236435 <https://reviews.llvm.org/D116832#3236435>, @pengfei wrote:

>> We have a huge issue with codegen churn causing pages of testcase changes in proposed merges and very little review is done on them.
>
> Can you show an example?

Sure.

Not so huge but all of the `ext` instructions just changed register names.  That's not what the patch is about so it's irrelevant to the test.
https://reviews.llvm.org/D115646

A little bigger, shows a bigger diff than is really warranted just because of register name changes.
https://reviews.llvm.org/D64174

I don't even know how to make sense of these test diffs.
https://reviews.llvm.org/D57367

That last one has more than register name changes going on, but the register name changes make it hard to pick out the changes that matter.  This is also an example of codegen tests that are just too large IMO.  Look at `test/CodeGen/X86/avg.ll` for example.  What is this actually testing?  The name isn't informative, there's no comment explaining what the test is and it's a big pile of asm.  Is all of that asm really necessary to check?  Maybe we only care about a very specific instruction sequence.  I don't know.  This is why I have follow-on patches to allow `update_llc_test_checks.py` to filter output and create a more focused asm test by only checking for specific patterns of asm.

This isn't even *that* bad of an example.  I've seen much worse, with hundreds of instructions in a test and diffs over basically the whole asm with the PR affecting many such tests.  I have no idea during review if the test changes are reasonable or if the submitter simply ran `update_llc_test_checks.py` and submitted the result.

I have had discussions with the people that originally created the `update_*` scripts and they were clear the intent was *not* to just blindly run it and submit the result without careful hand-editing.  Unfortunately, it has not turned out that way.  It makes sense to me to provide some tools to allow some semi-automatic narrowing of tests that will hopefully be used more than the amount of hand-editing that is done now (basically zero).

>> How so? I'm struggling to think of a time a regexp hid a real problem. Can you explain a bit more?
>
> Here is one example: https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/X86/distancemap.mir#L66
> The several `[[COPY6:%[0-9]+]]` are totally different things. I was fooled during the review. That's why I hate it.

Ah, yes, I can see how that would be an issue.  I'm understanding your concerns better, thanks.  So maybe replacing the register name with a FileCheck variable named by the original register isn't the best.  I could certainly alter this to uniquify names if it would help.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116832