This is an archive of the discontinued LLVM Phabricator instance.

[SLP]Fix cost of the broadcast buildvector/gather.
ClosedPublic

Authored by ABataev on Dec 21 2022, 1:40 PM.

Details

Summary

Need to include the cost of the initial insertelement to the cost of the
broadcasts. Also, need to adjust the cost of the gather/buildvector if
the element is ionserted into poison/undef vector.

Diff Detail

Event Timeline

ABataev created this revision.Dec 21 2022, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2022, 1:40 PM
ABataev requested review of this revision.Dec 21 2022, 1:40 PM
RKSimon added inline comments.Dec 22 2022, 2:42 AM
llvm/lib/Target/X86/X86TargetTransformInfo.h
152

How much work to avoid the default values on all the target impl? We don't do the same for Index.

ABataev updated this revision to Diff 484843.Dec 22 2022, 7:55 AM

Remove default values in target-specific implementations.

RKSimon added inline comments.Dec 23 2022, 7:53 AM
llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
1133

pre-commit whitespace cleanups separetely

llvm/test/Analysis/CostModel/X86/vector-insert-inseltpoison.ll
255

These costs are definitely not free - integer scalars still need to be transfered from gpr to xmm registers.

ABataev updated this revision to Diff 485138.Dec 23 2022, 8:59 AM

Address comments, fixed the cost of insertelement for integer constant.

RKSimon added inline comments.Jan 4 2023, 3:24 AM
llvm/lib/Target/X86/X86TargetTransformInfo.cpp
4346

constang -> constant

4347

Switch order in case Op1 is null:

if ( isa_and_nonnull<Constant>(Op1) && Op1->getType()->isIntegerTy())
llvm/test/Analysis/CostModel/X86/vector-insert-inseltpoison.ll
297

Something is still wrong - you have a more expensive cost to insert a i64 into [0] than [1] - for SSE4 they should both be cost = 1

ABataev updated this revision to Diff 486290.Jan 4 2023, 7:39 AM

Address comments

RKSimon accepted this revision.Jan 6 2023, 7:15 AM

LGTM - cheers

This revision is now accepted and ready to land.Jan 6 2023, 7:15 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a subscriber: fhahn.Jan 7 2023, 1:23 PM
fhahn added inline comments.
llvm/lib/Target/X86/X86TargetTransformInfo.cpp
4365–4369

Should this comment also be removed? It seems like the code has been removed?

ABataev added inline comments.Jan 7 2023, 1:36 PM
llvm/lib/Target/X86/X86TargetTransformInfo.cpp
4365–4369

No, it is not removed, the related code is all in the same lambda IsCheapPInsrPExtrInsertPS

fhahn added inline comments.Jan 10 2023, 4:11 AM
llvm/lib/Target/X86/X86TargetTransformInfo.cpp
4365–4369

Ah, but it documents the logic in the helper function now, which is used in multiple places. Should it not be moved there?

Also, shouldn't the 2 sentences be merged? At the moment, the wording seems to imply that pinsr/pextr XMM <-> GPR is relatively cheap on all targets AND insertps is relatively cheap on all targets., whereas it should be OR?

ABataev added inline comments.Jan 10 2023, 4:27 AM
llvm/lib/Target/X86/X86TargetTransformInfo.cpp
4365–4369

Ok, will move it to the lambda