As per title. This makes it easier to work onc hange that require "shotgun diffs" over the codebase.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Generating test output seems like a good change but I have no experience with it myself.
For most of these tests we don't care about the exact "no sve" output and thus have a single CHECK for the whole file. Is there a parameter that can be passed to update_llc_test_checks.py so those CHECK lines are not generated?
llvm/test/CodeGen/AArch64/sve-fixed-length-extract-subvector.ll | ||
---|---|---|
1–5 | Please can you -check-prefixes=CHECK,VBITS_GE_384 to the tests? |
Have you had chance to see if the extra NO_SVE lines can be removed. I really don't want these tests to require updates when SVE is not being used, whilst at the same time validating no SVE is used, which is what the current single NO_SVE-NOT lines verifies.
llvm/test/CodeGen/AArch64/sve-fixed-length-extract-subvector.ll | ||
---|---|---|
1–5 | Sorry I didn't get to respond before you made the change but I don't think VBITS_GE_384 makes sense for some (if not all) of the tests you're changing. The fixed length support it limited to power-of-two values (regardless of the actual target vector length) and so the likely real fix is for the RUN lines where only aarch64-sve-vector-bits-min is set to use VBITS_GE_256. You can see this when looking at the output with VBITS_EQ_256 and VBITS_GE_384 being identical. |
llvm/test/CodeGen/AArch64/sve-fixed-length-extract-subvector.ll | ||
---|---|---|
1–5 | How about this? ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py ; RUN: llc -aarch64-sve-vector-bits-min=128 < %s | FileCheck %s -check-prefixes=CHECK,NO_SVE ; RUN: llc -aarch64-sve-vector-bits-min=256 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_EQ_256 ; RUN: llc -aarch64-sve-vector-bits-min=384 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_EQ_256 ; RUN: llc -aarch64-sve-vector-bits-min=512 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512 ; RUN: llc -aarch64-sve-vector-bits-min=640 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512 ; RUN: llc -aarch64-sve-vector-bits-min=768 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512 ; RUN: llc -aarch64-sve-vector-bits-min=896 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512 ; RUN: llc -aarch64-sve-vector-bits-min=1024 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512,VBITS_GE_1024 ; RUN: llc -aarch64-sve-vector-bits-min=1152 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512,VBITS_GE_1024 ; RUN: llc -aarch64-sve-vector-bits-min=1280 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512,VBITS_GE_1024 ; RUN: llc -aarch64-sve-vector-bits-min=1408 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512,VBITS_GE_1024 ; RUN: llc -aarch64-sve-vector-bits-min=1536 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512,VBITS_GE_1024 ; RUN: llc -aarch64-sve-vector-bits-min=1664 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512,VBITS_GE_1024 ; RUN: llc -aarch64-sve-vector-bits-min=1792 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512,VBITS_GE_1024 ; RUN: llc -aarch64-sve-vector-bits-min=1920 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512,VBITS_GE_1024 ; RUN: llc -aarch64-sve-vector-bits-min=2048 < %s | FileCheck %s -check-prefixes=CHECK,SVE,VBITS_GE_512,VBITS_GE_1024,VBITS_GE_2048 |
I do not know if there is a way to simplify the NO_SVE case. Maybe an expert with utils/update_llc_test_checks.py could help?
To be honest these tests are just not a good fit for auto-generation, which is why we didn't use update_llc_test_checks.py originally. The best I could come up with is to disable the NO_SVE run line when running the script and then enable it afterwards. I do have the workings of a patch to add a --ignored-check-prefixes option to update_llc_test_checks.py but even then the decision as to which CHECK lines are emitted doesn't really match what we wanted to achieve. If autogeneration is preferable and you're happy to wait a week or two I think it'll be better if I just restructure all the SVE fixed length tests (essentially split them based on register size) and enable auto-generation at the same time. I think this is worth doing anyway to increase our test coverage and test parallelism as the existing layout means these tests are the most time consuming ones within the AArch64 directory.
That might be an acceptable solution to stop this blocking D127115 - by removing the '| FileCheck %s -check-prefix=NO_SVE' the update script will ignore those RUN lines. After that its just a question of manualy adding the FileCheck back and removing the 'NOTE:' top line - we have plenty of test files that are manually adjusted after the update script, removing the 'NOTE' just makes it clearer.
I'd possibly suggest replacing VBITS_EQ_256 with VBITS_GE_256 and adding it the 384 tests as well as the 256 tests (and get rid of the 384 specific checks - sorry my bad).
@paulwalker-arm Would that work or do you think the test refactor is imminent?
llvm/test/CodeGen/AArch64/sve-fixed-length-extract-subvector.ll | ||
---|---|---|
21–22 | If what we want to check is that this is not present, maybe we could simply replace the filecheck for NO_SVE with a grep that ensure ptrue isn't present. this will allow the other cases to be autogenerated without polluting the tests too much. Would that work? |
@paulwalker-arm Would that work or do you think the test refactor is imminent?
I'm happy with this. I do plan to work on the refactoring this week but if landing these changes unblocks other work then let's do that.
llvm/test/CodeGen/AArch64/sve-fixed-length-float-compares.ll | ||
---|---|---|
360–362 | Is this enforced by the update script? If not then please leave the comment style as was. There are a few other files with similar changes. |
llvm/test/CodeGen/AArch64/sve-fixed-length-float-compares.ll | ||
---|---|---|
362 | Has, this is leftovers from the cleanup I ran to get there. Let me restore these, and then I'll land this. Thanks. |
@deadalnix @paulwalker-arm Should sve-fixed-length-fp-vselect.ll be updated to match as well?
Sorry copy-paste mistake - I meant sve-fixed-length-fp-select.ll which has NO_SVE checks for codegen as well as a test for ptrue.
I see :)
There's many test files that this patch doesn't change. I just figured the patch only changed the currently problematic ones. In any case, for my refactoring I'll ensure all sve-fixed-length- files have auto-generated check lines.
Please can you -check-prefixes=CHECK,VBITS_GE_384 to the tests?