[PATCH] D92965: [NFC] Remove unused prefixes in llvm/test/CodeGen/X86

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D92965#2447504 <https://reviews.llvm.org/D92965#2447504>, @mtrofin wrote:

> In D92965#2445701 <https://reviews.llvm.org/D92965#2445701>, @pengfei wrote:
>
>>> I think you meant `--allow-unused-prefixes=true`.
>>
>> Yes, that's what I meant. Thanks for correct me.
>
> I was not familiar with update_llc_test_checks.py before this point, and, after digging a bit more, I understand @lebedev.ri 's concern - looks like quite a few (3K) of tests in CodeGen (14K or so) use it. So I wanted to understand why removing a prefix used nowhere in the test lead to an incorrect rewrite of some assertions.
>
> I believe I found the cause - ptal D93078 <https://reviews.llvm.org/D93078>. Once that lands, I'll redo this patch.
>
> Separately, am I understanding update_llc_test.checks.py's usage accurately - IIUC, it's basically this:
>
> - write new RUN line. Add, to FileCheck, a list of prefixes to check
> - write functions
> - run update_llc_test.checs.py => it will add the pertinent asserts to the function(s) added above
>
> I can understand one may add a larger list of labels than necessary when initially authoring a RUN line. Apologies if I'm missing a pain point not obvious to me (given lack of familiarity) - I'd argue that tidying that at this point should be quite straight forward: should the list contain prefixes that end up not being used, FileCheck gives that sub-list; if the outcome is surprising to the developer (i.e. they expected a label be actually used), they get an earlier heads-up than needing to scroll down to where the asserts were inserted; in either case, the fix should be quick (it's in the current change); and the benefit is that what the developer intended is actually checked for.

And this is not specific to codegen/`update_llc_test_checks.py` tests, the workflow is *exactly* the same for opt tests (`update_opt_test_checks.py`)...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92965