This is an archive of the discontinued LLVM Phabricator instance.

[VectorCombine][NFC][test] Supplement tests of the load-insert-store sequence
ClosedPublic

Authored by benshi001 on Aug 4 2023, 7:39 AM.

Diff Detail

Event Timeline

benshi001 created this revision.Aug 4 2023, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 7:39 AM
benshi001 requested review of this revision.Aug 4 2023, 7:39 AM
RKSimon added inline comments.Aug 4 2023, 9:14 AM
llvm/test/Transforms/VectorCombine/AArch64/load-insert-store.ll
8 ↗(On Diff #547204)

Was this generated by update_test_checks?

benshi001 marked an inline comment as done.Aug 4 2023, 7:48 PM
benshi001 added inline comments.
llvm/test/Transforms/VectorCombine/AArch64/load-insert-store.ll
8 ↗(On Diff #547204)

Yes. All checks were auto generated by update_test_checks, without any manual modifcation.

benshi001 marked an inline comment as done.Aug 4 2023, 7:50 PM
benshi001 updated this revision to Diff 547440.Aug 4 2023, 8:09 PM
benshi001 added inline comments.
llvm/test/Transforms/VectorCombine/AArch64/load-insert-store.ll
8 ↗(On Diff #547204)

I did not see CHECK-SAME in other test files, do we have to manually remove them ?

benshi001 updated this revision to Diff 547695.Aug 7 2023, 3:25 AM
benshi001 retitled this revision from [VectorCombine][NFC][test] Add tests for the 'foldSingleElementStore' transform to [VectorCombine][NFC][test] Suplement tests for the load-insert-store sequence.
benshi001 added a reviewer: MaskRay.
benshi001 set the repository for this revision to rG LLVM Github Monorepo.
benshi001 retitled this revision from [VectorCombine][NFC][test] Suplement tests for the load-insert-store sequence to [VectorCombine][NFC][test] Supplement tests of the load-insert-store sequence.
benshi001 edited the summary of this revision. (Show Details)
benshi001 added a comment.EditedAug 7 2023, 3:30 AM

I did not notice that there were already negative cases in the common file test/Transform/VectorCombine/load-insert-store.ll, so I only added target specific cases in my new patch version. These cases whether can be transformed depend on the TTI of each backend.

nikic added a comment.Aug 7 2023, 4:01 AM

Is this test coverage for an upcoming patch or just more thorough coverage for the existing transform?

llvm/test/Transforms/VectorCombine/AArch64/load-insert-store.ll
8 ↗(On Diff #547204)

This is the version 2 output of update_test_checks, which includes the function signature. Older tests don't.

benshi001 added a comment.EditedAug 7 2023, 4:30 AM

Is this test coverage for an upcoming patch or just more thorough coverage for the existing transform?

I plan to implement several TODOs in VectorCombine. And I hope these cases can be transformed in the future.

llvm/test/Transforms/VectorCombine/AArch64/load-insert-store.ll
8 ↗(On Diff #547204)

So we should keep it as what it is ?

nikic added inline comments.Aug 7 2023, 5:42 AM
llvm/test/Transforms/VectorCombine/AArch64/load-insert-store.ll
8 ↗(On Diff #547204)

Yes, you should keep it as is.

benshi001 marked 2 inline comments as done.Aug 7 2023, 6:28 PM
benshi001 updated this revision to Diff 549239.Aug 10 2023, 9:49 PM
benshi001 edited the summary of this revision. (Show Details)
MaskRay accepted this revision.Aug 13 2023, 9:18 PM
This revision is now accepted and ready to land.Aug 13 2023, 9:18 PM