Page MenuHomePhabricator

[AArch64][SVE] Add support for DestructiveBinaryImm DestructiveInstType
ClosedPublic

Authored by cameron.mcinally on Feb 24 2020, 10:10 AM.

Details

Summary

This is the fourth patch in a series to support prefixing destructive operations, with the MOVPRFX instruction, to build constructive operations. The previous patch is D73711.

Here we add support for the DestructiveBinaryImm DestructiveInstType.

This patch comes mostly from D71712, with some modifications for the upstream ImmArg changes to the shift intrinsics.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2020, 10:10 AM
cameron.mcinally planned changes to this revision.Fri, Mar 6, 2:34 PM

Just realized that this patch isn't complete. Changes to come...

Try again, this time including test file.

Also, make a small tweak for upstream changes.

sdesmalen added inline comments.Mon, Mar 9, 9:44 AM
llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1139–1149

The _zx suggests it covers the merging with undef case as well. Are those patterns you plan to add in this patch?

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

I think this change isn't needed anymore if let DestructiveInstType = DestructiveBinaryImm is added to line 4702.

cameron.mcinally marked an inline comment as done.

Don't define DestructiveInstType in sve_int_bin_pred_shift_imm_right<...>.

cameron.mcinally marked an inline comment as done and an inline comment as not done.Mon, Mar 9, 11:28 AM
cameron.mcinally added inline comments.
llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1139–1149

I could add those if you'd like. They're there in D71712.

I'm trying to keep the size of the patches to a minimum for reviewing purposes, so that's why these patches might seem short.

My general plan is to get all the framework (e.g. AArch64ExpandPseudoInsts.cpp changes) in first. That way adding patterns and tests later should be very easy to review. If you'd like me to upstream this in another way, I'm open to suggestions...

sdesmalen accepted this revision.Tue, Mar 17, 3:01 PM

LGTM!

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1139–1149

Okay that's fine with me, it was more that I was wondering whether we'd keep _zx in that case (as opposed to _z), but I'm happy to keep it as is for now.

This revision is now accepted and ready to land.Tue, Mar 17, 3:01 PM
This revision was automatically updated to reflect the committed changes.