Page MenuHomePhabricator

[COST]Improve cost model for shuffles in SLP.
Needs ReviewPublic

Authored by ABataev on Wed, Apr 14, 8:19 AM.

Details

Summary

Introduced masks where they are not added and improved target dependent
cost models to avoid returning of the incorrect cost results after
adding masks.

Diff Detail

Event Timeline

ABataev created this revision.Wed, Apr 14, 8:19 AM
ABataev requested review of this revision.Wed, Apr 14, 8:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Apr 14, 8:19 AM

Adding some AArch64 reviewers

fhahn added a subscriber: fhahn.Thu, Apr 15, 3:41 AM

Can you split off the target specific cost model changes? This makes it easier to track down potential regressions.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1408 ↗(On Diff #337463)

This should be moved up I think, before the scalable vector handling. It would also be good to have cost-model tests for those shuffles.

sdesmalen added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3712–3713

Can the finding of a more specific ShuffleKind be done by getShuffleCost when a Mask is given?
It seems a bit inconvenient to have to do that manually before calling this function.

RKSimon added inline comments.Thu, Apr 15, 4:39 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3712–3713

I made a similar comment on D100495 - we could do with a generic 'ShuffleKind' decoder helper function (e.g. in Analysis\VectorUtils.h) that everybody can use.

ABataev added inline comments.Fri, Apr 16, 6:30 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3712–3713

In this case, we need to update getShuffleCost function for all the targets to translate the mask. Is it ok if I'll do it in this patch?

sdesmalen added inline comments.Tue, Apr 20, 1:21 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3712–3713

There's already a few different changes in this patch (AArch64 cost-model, X86 cost-model and changes to the SLPVectorizer), so I think it makes more sense to do this in a separate patch.

Matt added a subscriber: Matt.Tue, Apr 20, 6:51 AM
ABataev added inline comments.Tue, Apr 20, 9:24 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1408 ↗(On Diff #337463)

There is getIntrinsicInstrCost-vector-reverse.ll already affected by the cost changes.

ABataev updated this revision to Diff 343123.Wed, May 5, 11:35 AM

Rebase + fixes