This is an archive of the discontinued LLVM Phabricator instance.

[llvm][sve] Lowering for VLS masked extending loads
ClosedPublic

Authored by DavidTruby on Aug 17 2021, 4:12 AM.

Details

Summary

This extends the custom lowering for extending loads on
fixed length vectors in SVE to support masked extending loads.

The existing tests for correct behaviour of masked extending loads
exhibit bad code generation due to the legalistaion of i1 vectors.
They have been left as-is and new tests have been added that do not
exhibit this behaviour.

Diff Detail

Event Timeline

DavidTruby created this revision.Aug 17 2021, 4:12 AM
DavidTruby requested review of this revision.Aug 17 2021, 4:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2021, 4:12 AM
paulwalker-arm added inline comments.Aug 17 2021, 4:30 AM
llvm/test/CodeGen/AArch64/sve-fixed-length-masked-loads.ll
656–657

Why is p0 hardcoded? Should this be [[PG0]]?

Also I think these should be VBITS_GE_512-DAG as the order of the loads can be switched,

658

This sets PG but no following instructions use it.

659–660

PG3 and PG2 are not set within this function.

Corrected tests to use the right PG values

Matt added a subscriber: Matt.Aug 17 2021, 9:24 AM
paulwalker-arm added inline comments.Aug 18 2021, 5:54 AM
llvm/test/CodeGen/AArch64/sve-fixed-length-masked-loads.ll
664

Is this necessary? As in, why not just change %ap to have the same types as %bp.

As an extension to this, once %a is a <32 x i16> do you need %b? I ask because the computation of %mask could be %mask = icmp eq <32 x i16> %a, zeroinitializer. This latter point probably goes for the whole file to be honest.

DavidTruby updated this revision to Diff 369908.Sep 1 2021, 5:43 AM

Modified new tests to use zeroinitializer rather than another loaded
pointer.

I will change all the remaining tests in this file and the equivalent
truncating store file to use this pattern instead if this is approved,
but I don't want to clutter this review or commit with unrelated test
changes so I'll do it as a separate commit.

llvm/test/CodeGen/AArch64/sve-fixed-length-masked-loads.ll
664

%ap needs to have the type <32 x i8> for us to test the masked extending load a few lines later; otherwise we're just performing a normal masked load which is already covered by other tests in this file.

paulwalker-arm added inline comments.Sep 1 2021, 9:08 AM
llvm/test/CodeGen/AArch64/sve-fixed-length-masked-loads.ll
664

Sure I get that but this test is testing a load that is sign extended, which corresponds to the last three lines of the test. The lines above the call to llvm.masked.load are just setting up the test and thus it's preferred to keep them as simple as possible.

Before the most recent change the function took the parameter <32 x i16>* %bp which can be used directly by the compare used to generate the mask and thus the zext can be removed.

DavidTruby updated this revision to Diff 371065.Sep 7 2021, 7:23 AM

Update tests to remove extra argument

DavidTruby updated this revision to Diff 371578.Sep 9 2021, 6:38 AM

@paulwalker-arm sorry I think I misunderstood your comments before,
I believe the form I've changed the tests to now should be more what
you were expecting?

paulwalker-arm accepted this revision.Sep 10 2021, 2:44 AM
This revision is now accepted and ready to land.Sep 10 2021, 2:44 AM
This revision was automatically updated to reflect the committed changes.
efriedma added inline comments.Sep 13 2021, 12:52 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10879

Do we need a check here to make sure we don't create illegal masked loads after legalization?