This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SME] Promote mask for masked load to a similar type size with load value.
ClosedPublic

Authored by dtemirbulatov on Aug 20 2023, 10:13 PM.

Details

Summary

I noticed that legalizer could keep an original mask type of masked load combined with sign/zero extend, but we have to extend the mask to a type similar to our combined load otherwise instruction selection could not lower the load.

Diff Detail

Event Timeline

dtemirbulatov created this revision.Aug 20 2023, 10:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2023, 10:13 PM
dtemirbulatov requested review of this revision.Aug 20 2023, 10:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2023, 10:13 PM
sdesmalen added inline comments.Aug 21 2023, 1:41 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
24963–24971

It is better to just call this Mask, because you're overwriting the "original" value of Mask with an extended version, so MaskOrig is a misnomer.

24964

Given that the number of elements between VT and the predicate mask should match, this can compare the size of element type directly (as opposed to comparing the size of the whole vector).

llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-masked-load.ll
409

This attribute is unnecessary for the test. The important thing is that this function is compiled for 'streaming-compatible SVE', which is already enforced by the flag passed to llc.

409

+sme is also not necessary for this test.

Matt added a subscriber: Matt.Aug 21 2023, 1:03 PM

Resolved comments.

dtemirbulatov marked 2 inline comments as done.Aug 22 2023, 4:40 AM
dtemirbulatov marked 2 inline comments as done.

Fixed typo.

Changed call ISD::isNormalLoad() to getExtensionType() != ISD::NON_EXTLOAD, since non normal load could be an indexing load.

sdesmalen added inline comments.Aug 22 2023, 6:59 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
24966

Is it necessary to check for the extension type? (i.e. if you remove this condition, do any tests fail?)

24967

nit: it's simpler to do VT.getScalarSizeInBits() > Mask.getValueType().getScalarSizeInBits()

dtemirbulatov added inline comments.Aug 22 2023, 7:03 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
24966

No tests failed, but I think it can only occur if the load is extending.

sdesmalen added inline comments.Aug 22 2023, 7:07 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
24966

Do you want to make it into an assert then?

dtemirbulatov marked 3 inline comments as done.

Addressed comments.

This revision is now accepted and ready to land.Aug 30 2023, 1:19 AM