This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Autogenerate sve-fixed-length tests. NFC
ClosedPublic

Authored by deadalnix on Jun 6 2022, 8:32 AM.

Details

Summary

As per title. This makes it easier to work onc hange that require "shotgun diffs" over the codebase.

Diff Detail

Unit TestsFailed

Event Timeline

deadalnix created this revision.Jun 6 2022, 8:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 8:32 AM
deadalnix requested review of this revision.Jun 6 2022, 8:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 8:32 AM
DavidSpickett resigned from this revision.Jun 6 2022, 8:40 AM

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?

RKSimon added a subscriber: RKSimon.
RKSimon added inline comments.
llvm/test/CodeGen/AArch64/sve-fixed-length-extract-subvector.ll
1–5

Please can you -check-prefixes=CHECK,VBITS_GE_384 to the tests?

deadalnix updated this revision to Diff 434910.Jun 7 2022, 12:15 PM

Use VBITS_GE_384

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.

nikic resigned from this revision.Jun 8 2022, 3:52 AM
RKSimon added inline comments.Jun 8 2022, 4:18 AM
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
deadalnix updated this revision to Diff 435202.Jun 8 2022, 9:09 AM

Use VBITS_GE_256

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?

paulwalker-arm added a comment.EditedJun 9 2022, 4:40 AM

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.

Matt added a subscriber: Matt.Jun 10 2022, 3:12 PM

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.

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?

deadalnix added inline comments.Jun 12 2022, 8:47 AM
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?

deadalnix updated this revision to Diff 436231.Jun 12 2022, 9:21 AM

Use not grep.

Rework the test a bit to improve the end result. Loop in a couple of extra test.

paulwalker-arm accepted this revision.Jun 13 2022, 3:47 AM

@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.

This revision is now accepted and ready to land.Jun 13 2022, 3:47 AM
deadalnix added inline comments.Jun 13 2022, 5:33 AM
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 updated this revision to Diff 436359.Jun 13 2022, 5:42 AM

Fixup erroneously remove ;

deadalnix updated this revision to Diff 436360.Jun 13 2022, 5:46 AM

More ; fixups

This revision was landed with ongoing or failed builds.Jun 13 2022, 5:50 AM
This revision was automatically updated to reflect the committed changes.

@deadalnix @paulwalker-arm Should sve-fixed-length-fp-vselect.ll be updated to match as well?

@deadalnix @paulwalker-arm Should sve-fixed-length-fp-vselect.ll be updated to match as well?

Sorry, I don't understand the question.

@deadalnix @paulwalker-arm Should sve-fixed-length-fp-vselect.ll be updated to match as well?

Sorry, I don't understand the question.

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.