This is an archive of the discontinued LLVM Phabricator instance.

[CostModel] Update default cost model for sadd/ssub overflow to match TargetLowering
ClosedPublic

Authored by craig.topper on Sep 29 2021, 10:45 AM.

Details

Summary

The expansion for these was updated in https://reviews.llvm.org/D47927 but the cost model was not adjusted.

I believe the cost model was also incorrect for the old expansion. The expansion prior to D47927 used 3 icmps using LHS, RHS, and Result to calculate theirs signs. Then 2 icmps to compare the signs. Followed by an And. The previous cost model was using 3 icmps and 2 selects. Digging back through git blame, those 2 selects used to be 2 icmps, but were changed in https://reviews.llvm.org/D90681

Diff Detail

Event Timeline

craig.topper created this revision.Sep 29 2021, 10:45 AM
craig.topper requested review of this revision.Sep 29 2021, 10:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2021, 10:45 AM
RKSimon added inline comments.Sep 29 2021, 12:27 PM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
1797–1798

Worth trying to specify the Args as well via the IntrinsicCostAttributes arg?

1798–1799

Can we specify the predicate (ICMP_SGT) as well? I've been meaning to deal with PR48337 for ages so X86 actually uses it...

craig.topper added inline comments.Sep 29 2021, 3:09 PM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
1797–1798

You mean to get the getArithmeticInstrCost calls? I don't see any arg operands for getCmpSelInstrCost.

Use correct ICMP predicate

RKSimon accepted this revision.Sep 30 2021, 2:25 AM

LGTM - cheers

llvm/include/llvm/CodeGen/BasicTTIImpl.h
1797–1798

OK - that's probably not going to help for add/sub anyhow.

llvm/test/Transforms/LoopVectorize/ARM/mve-saddsatcost.ll
11

This still looks suspiciously high

This revision is now accepted and ready to land.Sep 30 2021, 2:25 AM
This revision was landed with ongoing or failed builds.Sep 30 2021, 9:45 AM
This revision was automatically updated to reflect the committed changes.