Page MenuHomePhabricator

[SVE][LoopVectorize] Add masked load/store and gather/scatter support for SVE
ClosedPublic

Authored by david-arm on Jan 25 2021, 5:09 AM.

Details

Summary

This patch updates IRBuilder::CreateMaskedGather/Scatter to work
with ScalableVectorType and adds isLegalMaskedGather/Scatter functions
to AArch64TargetTransformInfo. In addition I've fixed up
isLegalMaskedLoad/Store to return true for supported scalar types,
since this is what the vectorizer asks for.

In LoopVectorize.cpp I've changed
LoopVectorizationCostModel::getInterleaveGroupCost to return an invalid
cost for scalable vectors, since currently this relies upon using shuffle
vector for reversing vectors. In addition, in
LoopVectorizationCostModel::setCostBasedWideningDecision I have assumed
that the cost of scalarising memory ops is infinitely expensive.

I have added some simple masked load/store and gather/scatter tests,
including cases where we use gathers and scatters for conditional invariant
loads and stores.

Diff Detail

Event Timeline

david-arm created this revision.Jan 25 2021, 5:09 AM
david-arm requested review of this revision.Jan 25 2021, 5:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2021, 5:09 AM
david-arm updated this revision to Diff 319023.Jan 25 2021, 8:24 AM
  • Only bail out of calculating group interleave costs if this is a reversed group.
  • Fixed some formatting issues.
david-arm updated this revision to Diff 319512.Jan 27 2021, 3:01 AM
  • Changed setCostBasedWideningDecision to use InstructionCost::getInvalid() when the cost is invalid.
  • Changed getInterleaveGroupCost to return an invalid cost for SVE.
fhahn edited reviewers, added: fhahn; removed: Florian.Jan 27 2021, 3:10 AM
fhahn added a subscriber: fhahn.
fhahn added inline comments.
llvm/test/Transforms/LoopVectorize/AArch64/sve-gather-scatter.ll
13

This shouldn't be necessary, LV will generate a minimum iteration check anyways.

16

are those blocks needed?

29

Is this necessary? Can we instead just load from a i64* or use i32 in the GEP? Same for the other tests.

david-arm added inline comments.Jan 27 2021, 3:16 AM
llvm/test/Transforms/LoopVectorize/AArch64/sve-gather-scatter.ll
13

Sure, I simply adapted these tests by dumping out the module before the vectoriser, i.e. I didn't hand craft the tests. :) I can remove them no problem.

16

Probably not. Again I didn't write these IR tests by hand - I dumped out the module before the vectoriser so that explains why these blocks are here. I can try to clean them up.

29

I deliberately left this in as the sext exposes a different code path in the vectoriser that was broken - see line 7463 in LoopVectorize.cpp. I'd like to have at least one test with a sext of i32 -> i64 so that the fix and test are in the same patch.

david-arm updated this revision to Diff 319548.Jan 27 2021, 6:12 AM
  • Tried to tidy up the tests a bit and remove unnecessary blocks.
david-arm marked 3 inline comments as done.Jan 27 2021, 6:12 AM
fhahn added inline comments.Jan 28 2021, 7:49 AM
llvm/test/Transforms/LoopVectorize/AArch64/sve-gather-scatter.ll
13

Thanks. This will make the life of anyone who has to adjust the tests in the future easier.

david-arm added inline comments.Jan 29 2021, 2:40 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7084

I just realised this TODO can probably be removed, which I can do before merging if that's ok?

kmclaughlin accepted this revision.Jan 29 2021, 5:52 AM

LGTM!

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
221

nit: I think this is the same check as we have at the top of isLegalMaskedLoadStore - could one of them maybe be changed so that they are consistent?

This revision is now accepted and ready to land.Jan 29 2021, 5:52 AM
david-arm updated this revision to Diff 320151.Jan 29 2021, 9:07 AM
david-arm edited the summary of this revision. (Show Details)
  • After @kmclaughlin's suggestion to unify the behaviour between the masked loads/stores and gathers/scatters I realised that we should be checking for scalar types isLegalMaskedLoad/Store in the same way as isLegalMaskedGather/Scatter does. This is what other targets do because the vectorizer queries support based on the scalar type.
  • Added tests that we vectorise with masked loads and stores.
david-arm retitled this revision from [SVE][LoopVectorize] Add gather/scatter support for SVE to [SVE][LoopVectorize] Add masked load/store and gather/scatter support for SVE.Jan 29 2021, 9:07 AM
david-arm marked an inline comment as done.
david-arm added inline comments.
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
221

That's a cracking bug you found @kmclaughlin! It turns out we weren't properly supporting masked loads and stores in the vectoriser either. Thanks!

kmclaughlin accepted this revision.Feb 1 2021, 10:23 AM

Thanks for making these changes @david-arm, LGTM

This revision was automatically updated to reflect the committed changes.
david-arm marked an inline comment as done.