This is an archive of the discontinued LLVM Phabricator instance.

[BasicTTIImpl] Fix getCastInstrCost for scalable vectors by querying for ElementCount.
ClosedPublic

Authored by sdesmalen on Feb 8 2021, 2:57 AM.

Details

Summary

This fixes an overly restrictive assumption that the vector is a FixedVectorType,
in code that tries to calculate the cost of a cast operation when splitting
a too-wide vector. The algorithm works the same for scalable vectors, so this
patch removes the cast<FixedVectorType>.

Diff Detail

Event Timeline

sdesmalen requested review of this revision.Feb 8 2021, 2:57 AM
sdesmalen created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2021, 2:57 AM

LGTM (though I can't speak to whether the costs make sense for AArch64)

david-arm added inline comments.Feb 9 2021, 6:53 AM
llvm/test/Analysis/CostModel/AArch64/cast.ll
711

Hi @sdesmalen, given you've added support in the cost function for scalable vectors of the type <vscale x 1 x ElTy> perhaps it's worth adding a test for that too? i.e.

%loadnxv1i32 = load <vscale x 1 x i32>, <vscale x 1 x i32>* undef
sdesmalen added inline comments.Feb 9 2021, 8:45 AM
llvm/test/Analysis/CostModel/AArch64/cast.ll
711

I could, but I'm not sure how helpful that is, because we can't currently vectorize <vscale x 1 x <eltty>>. For this the cost should return Invalid, but it just returns some value now. This is probably something to fix in a follow-up patch at some point. It also wouldn't help test the code-change in this patch.

david-arm accepted this revision.Feb 9 2021, 8:50 AM

LGTM!

llvm/test/Analysis/CostModel/AArch64/cast.ll
711

OK that's fine. I think it'd be good to fix getCastInstrCost to deal with the <vscale x 1 x> case properly then at some point, since at the moment it just lets them through. I guess this is a general problem for a lot of our cost model right now.

This revision is now accepted and ready to land.Feb 9 2021, 8:50 AM
This revision was landed with ongoing or failed builds.Feb 12 2021, 12:29 AM
This revision was automatically updated to reflect the committed changes.