Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I left about 12 tests when I fixed the prefixes issues using the script, because the changes on prefixes will resulted in conflict when re-generating them. I suspected if it is a bug in update_llc_test_checks.py, but I didn't find time to look through. Maybe we can add --allow-unused-prefixes=true for these failed tests as a workaround?
I actually just undid the re-generation changes, leaving just the unused prefix removal. I'm not sure what the pros/cons to re-generation would be - my reasoning was about "keeping this change scoped". The tests passed, though with a FileCheck with the flag flipped to not allow unused prefixes.
I'm afraid when pepole change something and need to update the tests, the conflict will confuse them and may spend more time to know what happened. At least we need to remove
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
, though I think adding --allow-unused-prefixes=true seems better.
I'd like to see other reviewers opinions.
I think you meant --allow-unused-prefixes=true.
+1, this is precisely why i think this whole --allow-unused-prefixes=false-by-default endeavor is a horrible idea.
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. 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.
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.
There are more then 6K tests. update_llc_test_checks.py is just one of the update scripts.
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
In fact, I usually assign one prefix to RUN line at the beginning and run update script. If there are duplications, I will add a common prefix for them.
For tests have many RUNs and functions, I may do round and round updates to find a relatively optimized combination. Some prefixes may become unused without aware of. Some attempts are not correct, but I'm aware of them since the script will warn the conflict.
And this is not specific to codegen/update_llc_test_checks.py tests, the workflow is *exactly* the same for opt tests (update_test_checks.py)...
| llvm/test/CodeGen/X86/extract-bits.ll | ||
|---|---|---|
| 456 ↗ | (On Diff #311936) | Please can you regenerate and pre-commit this file with the @PLT fix? Then rebase this patch. |
| llvm/test/CodeGen/X86/extract-bits.ll | ||
|---|---|---|
| 456 ↗ | (On Diff #311936) | done. I think we want also extract-lowbits.ll |
| llvm/test/CodeGen/X86/extract-lowbits.ll | ||
|---|---|---|
| 4 ↗ | (On Diff #311955) | This looks wrong - for instance - @bzhi32_a0 doesn't have any X86 checks..... |
| llvm/test/CodeGen/X86/extract-lowbits.ll | ||
|---|---|---|
| 4 ↗ | (On Diff #311955) | All this patch does is removes unused prefixes - so by the looks of it, @bzhi32_a0 didn't have X86 checks in the first place. Maybe because these other tests that use the X86 label produce conflicting outputs for @bzhi32_a0, case in which update_llc_test_checks does not produce an output for the function for that prefix (this is existing behavior). We probably need more fixing to update_llc_test_checks. I think you're saying that we want to ensure each function is tested by each RUN line? |