[X86] Unbreak LIT/FileCheck

[lebedev.ri]

Summary

The TLDR is that for the areas where autogenerated check lines
are predominant, and X86 is perhaps the biggest contender
due to large number of ISA's under test, it is totally normal
to have unused check prefixes, and the original concern that
the check prefix is unused because it is misspelled is non-applicable,
because, well, checklines are autogenerated.
See review for more discussions on the topic.

[RKSimon]

llvm/test/CodeGen/X86/lit.local.cfg

2

Do we need this?

[lebedev.ri]

llvm/test/CodeGen/X86/lit.local.cfg

2

Need - no. I only copied what other lit.local.cfg do. Should i drop it?

[xbolva00]

If a new test needs, it can specify ‘ --allow-unused-prefixes’ explicitly, no? If we apply patch like this one to more and more folders, than original work was a wasted effort.

D95849 was a tragedy/sabotage that should have never happened

Too strong words I would say. Read code of conduct. You can disagree - give exact facts instead of calling someone’s (in this case @MaskRay) work a project sabotage. Plus, I see no objections from you in linked phab review and in rfc thread (and year passed…).

[lebedev.ri]

Drop python syntax hint.

[lebedev.ri]

In D123897#3455358, @xbolva00 wrote:

<...>

Hello. I'm not falling for this trap again.

[xbolva00]

If a new test needs, it can specify ‘ --allow-unused-prefixes’ explicitly, no? If we apply patch like this one to more and more folders, than original work was a wasted effort.

So if you are not willing to answer legit question and consider / answer / discuss other approaches, then …

[mtrofin]

In D123897#3455358, @xbolva00 wrote:

If a new test needs, it can specify ‘ --allow-unused-prefixes’ explicitly, no? If we apply patch like this one to more and more folders, than original work was a wasted effort.

In these cases - where whole directories are predominantly auto-generated tests - the idea was to use the approach in this patch, for example Transforms/Attributor/lit.local.cfg (where basically all are autogenerated). For CodeGen/X86, more than half of the tests have the word 'autogenerated' in them.

Maybe we should just use the word autogenerated in the header of the .ll file and make FileCheck automatically switch to --allow-unused-prefixes in that case?

D95849 was a tragedy/sabotage that should have never happened

Too strong words I would say. Read code of conduct. You can disagree - give exact facts instead of calling someone’s (in this case @MaskRay) work a project sabotage. Plus, I see no objections from you in linked phab review and in rfc thread (and year passed…).

[xbolva00]

Maybe we should just use the word autogenerated in the header of the .ll file and make FileCheck automatically switch to --allow-unused-prefixes in that case?

Possibly yes, or other way:
As an updater python script emits first line (NOTE: autogenerated by..) , it could also append “—allow-unused-prefixes” ?

[xbolva00]

And https://reviews.llvm.org/D94744 used explicit approach.

[lebedev.ri]

In D123897#3455386, @mtrofin wrote:

In D123897#3455358, @xbolva00 wrote:

If a new test needs, it can specify ‘ --allow-unused-prefixes’ explicitly, no? If we apply patch like this one to more and more folders, than original work was a wasted effort.

In these cases - where whole directories are predominantly auto-generated tests - the idea was to use the approach in this patch, for example Transforms/Attributor/lit.local.cfg (where basically all are autogenerated). For CodeGen/X86, more than half of the tests have the word 'autogenerated' in them.

Maybe we should just use the word autogenerated in the header of the .ll file and make FileCheck automatically switch to --allow-unused-prefixes in that case?

I'm only asking other X86 contributors whether they had enough and want to go back to pre---allow-unused-prefixes FileCheck.
As for the global scope of things - I don't know, the damage has already been done. I guess i'd personally recommend starting an RFC to at least flip the default back to the sensible one.

D95849 was a tragedy/sabotage that should have never happened

Too strong words I would say. Read code of conduct. You can disagree - give exact facts instead of calling someone’s (in this case @MaskRay) work a project sabotage. Plus, I see no objections from you in linked phab review and in rfc thread (and year passed…).

[MaskRay]

You need evidence to back up your statement. How is writing new llvm/test/CodeGen/X86 tests impossible now? Are there concrete examples when --allow-unused-prefixes=false makes the test less appealing in a way which can't be mitigated?
I think some folks may be interested to improve update_llc_test_checks.py to mitigate the effect while retaining the error checking effect. There may also be users who want the new effect even with update_llc_test_checks.py generated tests.

[mtrofin]

Added David back - he had some concerns and a good (I think) suggestion. Could this patch do his “UTC_ARGS: —allow-unused-prefixes” suggestion instead? Seems it wouldn't be any bigger of a change, and I think everybody would have their concerns addressed.

[lebedev.ri]

In D123897#3455421, @MaskRay wrote:

You need evidence to back up your statement.

Let's unflip the table. Why do you believe the current default is right, for X86 tests?, for FileCheck in general?
If you like having broken default, that's up to you, but i'm confident that that not many agree with you on this.
I'm only fixing the X86 story that affects me.

How is writing new llvm/test/CodeGen/X86 tests impossible now?

It is especially egregiously fundamentally by-design incompatible with the way
multi-prefix output deduplication works for autogenerated checklines.

It is *normal* to have unused check prefixes afterwards, yet now i'm forced to remove them,
and guess what, one day after some codegen change the prefix would become used,
yet it is not there anymore, and the check lines will go missing.

The last drop in the bucket that prompted me to submit this was that i refreshed X86 test coverage for interleaved load/store patters,
which took me ~30 minutes, and then i spend 20 minutes trying to reconcile with FileCheck on 1/8'th of the new tests.
It's a monumental waste of time, that only hurts, both in short run and long run.

Are there concrete examples when --allow-unused-prefixes=false makes the test less appealing in a way which can't be mitigated?

Again, anything to that effect is fundamentally wrong question, akin telling people to stop using the update_llc_test_checks.py family of scripts.

I think some folks may be interested to improve update_llc_test_checks.py to mitigate the effect while retaining the error checking effect. There may also be users who want the new effect even with update_llc_test_checks.py generated tests.

[lebedev.ri]

In D123897#3455425, @mtrofin wrote:

Added David back - he had some concerns and a good (I think) suggestion. Could this patch do his “UTC_ARGS: —allow-unused-prefixes” suggestion instead? Seems it wouldn't be any bigger of a change, and I think everybody would have their concerns addressed.

Everybody except me? :)

[xbolva00]

In D123897#3455394, @xbolva00 wrote:

And https://reviews.llvm.org/D94744 used explicit approach.

Use same approach here?

[mtrofin]

In D123897#3455433, @lebedev.ri wrote:

In D123897#3455425, @mtrofin wrote:

Added David back - he had some concerns and a good (I think) suggestion. Could this patch do his “UTC_ARGS: —allow-unused-prefixes” suggestion instead? Seems it wouldn't be any bigger of a change, and I think everybody would have their concerns addressed.

Everybody except me? :)

My understanding is that his proposal will make any test generated with one of the auto-generators automatically enrolled into --allow-unused-prefixes. Wouldn't that address your scenario?

[lebedev.ri]

In D123897#3455434, @xbolva00 wrote:

In D123897#3455394, @xbolva00 wrote:

And https://reviews.llvm.org/D94744 used explicit approach.

Use same approach here?

I'm sorry, i think i'm having trouble understanding your messages.
Could you please more words and longer, better formulated, phrases?

[lebedev.ri]

In D123897#3455435, @mtrofin wrote:

In D123897#3455433, @lebedev.ri wrote:

In D123897#3455425, @mtrofin wrote:

Added David back - he had some concerns and a good (I think) suggestion. Could this patch do his “UTC_ARGS: —allow-unused-prefixes” suggestion instead? Seems it wouldn't be any bigger of a change, and I think everybody would have their concerns addressed.

Everybody except me? :)

My understanding is that his proposal will make any test generated with one of the auto-generators automatically enrolled into --allow-unused-prefixes. Wouldn't that address your scenario?

1. It's a workaround for the case of unchanged default. I'm interested in fixing the default, not workarounding it.
2. UTC_ARGS controls the default arguments for the update_llc_test_checks.py family of checks. Can you please explain how it would happen to affect the FileCheck? Should update_llc_test_checks.py rewrite the RUN lines? Or would FileCheck itself need to be modified to somehow look for it?

[xbolva00]

2. Updater scripts could just modify RUN lines and append “—allow-unused-prefixes” in “FileCheck” invocation part

[xbolva00]

The last drop in the bucket that prompted me to submit this was that i refreshed X86 test coverage for interleaved load/store patters, which took me ~30 minutes, and then i spend 20 minutes trying to <…>

A few seconds to change RUN lines and add “—allow-unused-prefixes” explicitly.

[MaskRay]

In D123897#3455437, @lebedev.ri wrote:

In D123897#3455435, @mtrofin wrote:

In D123897#3455433, @lebedev.ri wrote:

In D123897#3455425, @mtrofin wrote:

Added David back - he had some concerns and a good (I think) suggestion. Could this patch do his “UTC_ARGS: —allow-unused-prefixes” suggestion instead? Seems it wouldn't be any bigger of a change, and I think everybody would have their concerns addressed.

Everybody except me? :)

My understanding is that his proposal will make any test generated with one of the auto-generators automatically enrolled into --allow-unused-prefixes. Wouldn't that address your scenario?

1. It's a workaround for the case of unchanged default. I'm interested in fixing the default, not workarounding it.

If you want to flip the default again, you can start a new thread on discourse and leave a comment on https://discourse.llvm.org/t/rfc-filecheck-dis-allowing-unused-prefixes/56806
Note that the first message in the thread mentioned "1579 tests have this property." that was a significant portion of the testsuite.
I object to the flip (to true) since it will leave llvm-project's testsuite into the historical state that check prefixes easily got wrong.

You probably need to re-frame the problem you observed in more words.

2. UTC_ARGS controls the default arguments for the update_llc_test_checks.py family of checks. Can you please explain how it would happen to affect the FileCheck? Should update_llc_test_checks.py rewrite the RUN lines? Or would FileCheck itself need to be modified to somehow look for it?

I agree UTC_ARGS is currently only recognized by update*test_checks.py. FileCheck doesn't support it. If there is indeed a significant need, I think we can teach FileCheck a similar marker (if this is considered better than changing all FileCheck lines in the file to specify --allow-unused-prefixes)

[lebedev.ri]

Once again, let's cut on the bikeshedding here, please.
This is only affects X86 codegen tests, all of which should be autogenerated in the first place, so the check prefix can not be wrong in the first place.

[mtrofin]

In D123897#3455437, @lebedev.ri wrote:

In D123897#3455435, @mtrofin wrote:

In D123897#3455433, @lebedev.ri wrote:

In D123897#3455425, @mtrofin wrote:

Added David back - he had some concerns and a good (I think) suggestion. Could this patch do his “UTC_ARGS: —allow-unused-prefixes” suggestion instead? Seems it wouldn't be any bigger of a change, and I think everybody would have their concerns addressed.

Everybody except me? :)

My understanding is that his proposal will make any test generated with one of the auto-generators automatically enrolled into --allow-unused-prefixes. Wouldn't that address your scenario?

1. It's a workaround for the case of unchanged default. I'm interested in fixing the default, not workarounding it.

I think there are 2 scenarios, please correct me if I misunderstand them (esp. the first):

• I use autogen-ing script, and rely on 'unused prefixes', so I want to just write/update my .ll, run the autogenerator, and be done.
• I write tests manually, and I can make mistakes (like mean to use a prefix but actually mistakenly not), which I would like to have auto-detected

There are 29164 .ll files under llvm/test, out of which 12449 have the word 'autogenerated' in them, so less than half. If the default were flipped, the onus would be on the manually generated ones to flip it on, which is likely to fail us - people forget, etc. The autogenerated ones both appear to be the main users of allowing unused prefixes, and in the position of opting in to their desired behavior, transparent to the user. That's why I think that 1) leaving the default as-is, and 2) having the autogenerated cases transparently opt in to the behavior they want is win-win.

2. UTC_ARGS controls the default arguments for the update_llc_test_checks.py family of checks. Can you please explain how it would happen to affect the FileCheck? Should update_llc_test_checks.py rewrite the RUN lines? Or would FileCheck itself need to be modified to somehow look for it?

I'm learning on the fly about UTC_ARGS, so maybe it's not the right tool for the job; what I mean to say here is captured above, i.e. a feature of the update scripts that automatically opts generated tests to --allow-unused-prefixes (basically, giving the user the transparent experience).

[lebedev.ri]

In D123897#3455496, @mtrofin wrote:

In D123897#3455437, @lebedev.ri wrote:

In D123897#3455435, @mtrofin wrote:

In D123897#3455433, @lebedev.ri wrote:

In D123897#3455425, @mtrofin wrote:

Added David back - he had some concerns and a good (I think) suggestion. Could this patch do his “UTC_ARGS: —allow-unused-prefixes” suggestion instead? Seems it wouldn't be any bigger of a change, and I think everybody would have their concerns addressed.

Everybody except me? :)

My understanding is that his proposal will make any test generated with one of the auto-generators automatically enrolled into --allow-unused-prefixes. Wouldn't that address your scenario?

1. It's a workaround for the case of unchanged default. I'm interested in fixing the default, not workarounding it.

I think there are 2 scenarios, please correct me if I misunderstand them (esp. the first):

• I use autogen-ing script, and rely on 'unused prefixes', so I want to just write/update my .ll, run the autogenerator, and be done.
• I write tests manually, and I can make mistakes (like mean to use a prefix but actually mistakenly not), which I would like to have auto-detected

There are 29164 .ll files under llvm/test, out of which 12449 have the word 'autogenerated' in them, so less than half. If the default were flipped, the onus would be on the manually generated ones to flip it on, which is likely to fail us - people forget, etc. The autogenerated ones both appear to be the main users of allowing unused prefixes, and in the position of opting in to their desired behavior, transparent to the user.

This sounds correct to me.

That's why I think that 1) leaving the default as-is, and 2) having the autogenerated cases transparently opt in to the behavior they want is win-win.

99.9% of new tests in this directory are autogenerated, thus the diff in question. I find the rest of the questions to be bikeshedding.

2. UTC_ARGS controls the default arguments for the update_llc_test_checks.py family of checks. Can you please explain how it would happen to affect the FileCheck? Should update_llc_test_checks.py rewrite the RUN lines? Or would FileCheck itself need to be modified to somehow look for it?

I'm learning on the fly about UTC_ARGS, so maybe it's not the right tool for the job; what I mean to say here is captured above, i.e. a feature of the update scripts that automatically opts generated tests to --allow-unused-prefixes (basically, giving the user the transparent experience).

[mtrofin]

In D123897#3455488, @lebedev.ri wrote:

Once again, let's cut on the bikeshedding here, please.
This is only affects X86 codegen tests, all of which should be autogenerated in the first place, so the check prefix can not be wrong in the first place.

My $0.02, personally I think this patch is fine, what prompted me to propose an alternative is that CodeGen/X86 has 2483 tests with 'autogenerated' out of 4136 total; so with an eye to those ~1K manual ones, got me thinking, maybe there's a way we can easily get everything we want? If that's not as easily achievable as this patch at this moment, we can punt it (i.e. let's go ahead with this patch to unblock Roman - if he's got a bunch of tests needing updating, if I were him, I'd also hate having to preen each of them; and then we can follow up with a patch that fixes the autogen-ers and removes this lit.local.cfg)

How's this sound?

[nikic]

Sounds reasonable to me. I do think that allowing unused prefixes for generated tests would be valuable in general, but this is also a reasonable solution in the meantime. X86 tests in particular probably benefit most, because these tests routinely use a rather large matrix of different vector extensions.

[lebedev.ri]

Thanks everyone, i do believe we now have a rough understanding/consensus.
Does anyone feel like stamping the patch?

In D123897#3455499, @mtrofin wrote:

<...>

I think it may be interesting to fix FileCheck to default to --allow-unused-prefixes
if the first line of the test contains NOTE: Assertions have been autogenerated by comment,
but given that X86/ effectively does not welcome new manually-written checklines,
i believe the X86-specific default change/unbreak is a correct thing to do.

[llvm-commits]

Could we hold off pushing this forwards please for a couple of days? Easter
weekend is time off work for many people.

• msg-17440-550.txt163 BDownload

[RKSimon]

Sorry for the delay - as had been mentioned its Easter break in the UK, so I'm not around much for the next few days.

The number of permutations of X86 targets / attributes / features means its one of the hardest cases to determine an optimal set/sequence of check-prefixes for multiple, diverse RUN commands. I want to be able to run the same file for all of the permutations, with the understanding that the codegen is going to keep changing (and hopefully improving) and the CHECKs will need easily regenerating, I don't want to end up trying to keep separate test files in sync apart from their RUN. For that reason alone, I think allowing unused prefixes in the X86 codegen directory makes sense until we can clean up some of the other problems we currently have with the lit + update scripts (see below).

I've hit the same levels of frustration as Roman when trying to add new tests or alter codegen; something that is a particular problem is that lit raises errors for something as benign as an unused prefix (and only reports the first error), but neither it or the update scripts barely notice when a particular RUN has no check coverage for a particular function - e.g when I've altered SSE4.1 codegen but we only had a common SSE check (because SSE2/SSSE3/SSE41 had been unused), and we get no warning that checks for SSE2/SSSE3/SSE41 RUNs all disappear - a few years ago the update script usually managed to recognise when a match for a given function+RUN failed (it only occasionally failed depending on the order of prefixes), but we have lost that.

We seem to have gotten to a stage where the update scripts have lost their ability to assist with developing test coverage - I accept that lit tests should be optimized for testing of the existing tests, and should treat unused/unknown prefixes as errors by default - but the update scripts need better warning of missing checks, automatically adding UTC_ARGS --allow-unused-prefix tags if we do have unused prefixes, warning when multiple prefixes often/always have the same matches, and a more complete output of warnings encountered, (not just the first), are all things we need to address soon.

[tstellar]

@lebedev.ri This commit message is not appropriate. Please change it in before committing to the repo and also update this review with something else.

[lebedev.ri]

In D123897#3456542, @tstellar wrote:

@lebedev.ri This commit message is not appropriate. Please change it in before committing to the repo and also update this review with something else.

Presumably the updated description has seen enough censorship?

[aaron.ballman]

In D123897#3456741, @lebedev.ri wrote:

In D123897#3456542, @tstellar wrote:

@lebedev.ri This commit message is not appropriate. Please change it in before committing to the repo and also update this review with something else.

Presumably the updated description has seen enough censorship?

I have zero skin in this game (I only noticed this thread by accidentally clicking on it in my inbox), but no. Please rework the summary for this patch to not be in rather obvious violation of the community's code of conduct.

[aaron.ballman]

Personally, i believe that change have been totally beneficial, and legally mandated by our fully functional RFC process, so i can't really say that the fact that it destroyed a number of carefully thought-through check prefixes was damaging to the project.

This is no more acceptable than the last one, @lebedev.ri.

[tstellar]

I will try to be more specific:

Paragraph 1:

• This contains a non-technical criticism of someone else's work and provides no useful information about the commit. Please remove.

Paragraph 2:

• LGTM

Paragraph 3:

• I don't understand exactly what is being said here, but the words 'destroyed' and 'damaging' seem overly aggressive. Please drop this paragraph too. The 2nd paragraph on its own is a good enough commit message.

[lebedev.ri]

In D123897#3456947, @tstellar wrote:

I will try to be more specific:

Paragraph 1:

• This contains a non-technical criticism of someone else's work and provides no useful information about the commit. Please remove.

Paragraph 2:

• LGTM

Paragraph 3:

• I don't understand exactly what is being said here, but the words 'destroyed' and 'damaging' seem overly aggressive. Please drop this paragraph too. The 2nd paragraph on its own is a good enough commit message.

Apologies, it seems like the feedback here arrives much faster than usual, so there are merge conflicts.
If the current description is still seen as bad, here's a black marker, feel free to fine-tune it.
Thanks!

[probinson]

In D123897#3456962, @lebedev.ri wrote:

Apologies, it seems like the feedback here arrives much faster than usual, so there are merge conflicts.
If the current description is still seen as bad, here's a black marker, feel free to fine-tune it.
Thanks!

I've applied @tstellar 's suggestions to the review description. Please make sure this propagates to the commit message.

[probinson]

Offhand, it seems like the next step would be a patch to make the update scripts apply --allow-unused-prefixes to all FileCheck commands. Then all autogenerated scripts would (eventually) DTRT, and this patch could then be reverted.

Finding mistakes in hand-written tests (which are the majority of all lit tests) is important, and so the FileCheck default is correct IMO.

[RKSimon]

ping @tstellar

[lattner]

This seems reasonable to me

[tstellar]

@lebedev.ri are you still working on this or should someone else commandeer the revision?

[lebedev.ri]

Alright. Well. I'm not really sure i should,
but it is possible that the time has come
to somewhat come back
from this reduced mental strain hiatus.

<...>:
• The commit message is approved by a member of the Code of Conduct response team.

I do believe that this has been addressed.
(i always use arc patch, so what is in phab, will be in the git log.)

• All previous reviewer questions have been answered.

Assuming that we agree that the disscussion here did converge
on the need for this LIT change itself, i am under impression
that all the questions were answered. (audience can correct me)

• All active approvers, or previous reviewers that requested changes have accepted the review.

That would be @tstellar - i believe those concerns were strictly about the textual description of the change, and those were addressed since.

[nikic]

I'm not sure we still need this change after https://reviews.llvm.org/D124306.

llvm/test/CodeGen/X86/lit.local.cfg

+from lit.llvm.subst import ToolSubst
+
 if not 'X86' in config.root.targets:
     config.unsupported = True
+
+fc = ToolSubst('FileCheck', unresolved='fatal')
+config.substitutions.insert(0, (fc.regex, 'FileCheck --allow-unused-prefixes'))