This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE]Add cost model for broadcast shuffle
ClosedPublic

Authored by CarolineConcatto on Jan 28 2021, 1:05 AM.

Details

Summary

This patch adds a cost model for SK_Broadcast in
AArch64TTIImpl::getShuffleCost with scalable vector.
Without this patch, the scalable vector type relies on BasicTTIImpl cost
implementation and assert.

Diff Detail

Unit TestsFailed

Event Timeline

CarolineConcatto requested review of this revision.Jan 28 2021, 1:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2021, 1:05 AM
sdesmalen added inline comments.
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1194

Why did you remove this condition?

1246

Can't these be moved into the table above? (ShuffleTbl)

llvm/test/Analysis/CostModel/AArch64/sve-shuffle-broadcast.ll
102

s/slapt/broadcast/ (same for tests below)

-add original if statement
-move the table cost for scalable types to ShuffleTbl

-replace test name @slapt_v4f32 by @broadcast_v4f32(

CarolineConcatto marked 3 inline comments as done.Jan 28 2021, 3:10 AM
CarolineConcatto added inline comments.
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1194

I don't see any reason why we need this if here.
IMO it is a redundancy that I think should be removed if we have more then one cost table.
But as we are merging the cost for scalable vectors with fixed vectors I am leaving this if as it was.

1246

We can, but I thought that having one table cost for fixed vector and one for scalable makes the code clear.

david-arm added inline comments.Jan 28 2021, 5:41 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1241

I think we also need one for the type MVT::nxv8bf16 here.

1241

nit: Whitespace change

llvm/test/Analysis/CostModel/AArch64/sve-shuffle-broadcast.ll
12

Hi Carol, this is just a suggestion, but a reviewer (David Green) on one of my recent cost model patches suggested that we could do all tests in one function, i.e.

define void @broadcast_legal() {
%0 = shufflevector <vscale x 16 x i8> %v, <vscale x 16 x i8> poison, <vscale x 16 x i32> zeroinitializer
%1 = shufflevector <vscale x 32 x i8> %v, <vscale x 32 x i8> poison, <vscale x 32 x i32> zeroinitializer
...
}

and so on. I found it really useful as all the CHECK lines can go in the same function and it reduces the test size. He mentioned that in my patch here - https://reviews.llvm.org/D95039. It means we can reduce the number of functions and number of unnecessary checks for the "ret void" instruction.

CarolineConcatto marked 2 inline comments as done.
  • simplify sve broadcast test
CarolineConcatto marked an inline comment as done.
  • add bfloat test and support
  • correct style
CarolineConcatto marked 2 inline comments as done.Jan 29 2021, 8:24 AM

@Thank you all for the suggestion.
I have added the support for MVT::nxv8bf16 and simplify the test.

david-arm accepted this revision.Feb 1 2021, 1:12 AM

LGTM with nit addressed!

llvm/test/Analysis/CostModel/AArch64/sve-shuffle-broadcast.ll
12

nit: It looks like '%v' isn't used anymore here so can delete?

This revision is now accepted and ready to land.Feb 1 2021, 1:12 AM

-remove non used parameter from the test function broadcast

CarolineConcatto marked an inline comment as done.Feb 3 2021, 1:21 AM
This revision was landed with ongoing or failed builds.Feb 3 2021, 1:56 AM
This revision was automatically updated to reflect the committed changes.