This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Add some logical operation DestructiveBinaryComm patterns
ClosedPublic

Authored by Allen on Apr 21 2022, 7:14 PM.

Details

Summary

Add DestructiveBinaryComm* patterns for ORR, EOR, AND and BIC.
The above instructions requires that the source and destination registers are
equal, so use movprfx should be beneficial to performance.

Diff Detail

Event Timeline

Allen created this revision.Apr 21 2022, 7:14 PM
Allen requested review of this revision.Apr 21 2022, 7:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 7:14 PM

This patch seems sensible to me! I just had a couple of minor comments.

llvm/lib/Target/AArch64/SVEInstrFormats.td
2773

I don't think we need these because there aren't any reverse logical operations.

2776

Again, I don't think we need any of these for the logical operations.

Allen added inline comments.Apr 22 2022, 1:13 AM
llvm/lib/Target/AArch64/SVEInstrFormats.td
2773

I add these arguments of string revname="" and bit isOrig=0 as the API SVEPseudo2Instr need ?

2776

Thanks very much , can you help to detailly point out what's "these" ? I may don't get your idea?

david-arm added inline comments.Apr 22 2022, 1:18 AM
llvm/lib/Target/AArch64/SVEInstrFormats.td
2776

Sorry I wasn't being very clear - I was relying upon my comment highlighting the right part of the code! I just mean that the ", SVEInstr2Rev<...>" bits are unnecessary I think? We don't have any reverse logical operations so perhaps we can just delete it? If so, you can also just delete the 'revname' and 'isOrig' arguments passed to the multiclass as well.

Allen updated this revision to Diff 424414.Apr 22 2022, 2:19 AM

delete unnecessary bits SVEInstr2Rev<...> according review

paulwalker-arm added inline comments.Apr 22 2022, 4:09 AM
llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
342

If you don't mind indulging my OCD I've tried to ensure the pseudo instructions are defined after the real ones so can you move the definitions for ORR_ZPmZ..BIC_ZPmZ to the bottom of this block so they're before the following UseExperimentalZeroingPseudos block.

354

BIC (i.e. A & ~B) is not a commutative operation and so should use DestructiveBinary.

Allen updated this revision to Diff 424441.Apr 22 2022, 4:51 AM
Allen marked 2 inline comments as done.

use DestructiveBinary as BIC (i.e. A & ~B) is not a commutative operation

paulwalker-arm accepted this revision.Apr 22 2022, 5:06 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
466

Not quite the placement I had in mind but it doesn't matter.

This revision is now accepted and ready to land.Apr 22 2022, 5:06 AM
This revision was landed with ongoing or failed builds.Apr 22 2022, 5:31 AM
This revision was automatically updated to reflect the committed changes.
Allen marked an inline comment as done.
thakis added a subscriber: thakis.Apr 22 2022, 6:43 AM

Looks like this broke tests: http://45.33.8.238/linux/74409/step_12.txt

Please take a look and revert for now if it takes while to fix.

Allen added a comment.Apr 22 2022, 5:51 PM

Thanks!

@thakis sorry for this patch
@paulwalker-arm thanks very much for helping fix this issue

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
342

Apply your review, thanks

354

good catch, thanks for your carefully review

llvm/lib/Target/AArch64/SVEInstrFormats.td
2776

Thanks very much for explaining.