This is an archive of the discontinued LLVM Phabricator instance.

[TTI] Add generic UADDSAT/USUBSAT and UADDO/USUBO costs
ClosedPublic

Authored by RKSimon on Jan 18 2019, 7:05 AM.

Details

Summary

Add generic costs calculation for UADDSAT/USUBSAT intrinsics, this fallbacks to using generic costs for uadd_with_overflow/usub_with_overflow + a select.

Added x86 scalar uadd_with_overflow/usub_with_overflow costs.

Diff Detail

Repository
rL LLVM

Event Timeline

nikic added inline comments.Jan 18 2019, 11:33 AM
include/llvm/CodeGen/BasicTTIImpl.h
1278 ↗(On Diff #182509)

Should we omit the icmp cost for the scalar case, on the assumption that it will be handled as an overflow flag instead?

test/Analysis/CostModel/X86/arith-usat.ll
313 ↗(On Diff #182509)

Not directly related to this change, but the current icmp cost model for X86 doesn't distinguish comparison types, so it assigns the same cost to signed and unsigned vector comparisons, even though the latter only exist since AVX512. The cost here should probably be 5 to account for the sign flips.

RKSimon marked an inline comment as done.Jan 18 2019, 1:48 PM
RKSimon added inline comments.
test/Analysis/CostModel/X86/arith-usat.ll
313 ↗(On Diff #182509)
RKSimon marked 2 inline comments as done.Jan 18 2019, 1:55 PM
RKSimon added inline comments.
include/llvm/CodeGen/BasicTTIImpl.h
1278 ↗(On Diff #182509)

Maybe just add those as x86 entries for now unless you want to add cost support for add/sub overflow/carry intrinsics as well?

test/Analysis/CostModel/X86/arith-usat.ll
313 ↗(On Diff #182509)

Vector select costs are missing as well.

nikic added inline comments.Jan 19 2019, 1:58 AM
include/llvm/CodeGen/BasicTTIImpl.h
1278 ↗(On Diff #182509)

Having X86 entries would be fine as well, though I think that the availability of overflow flags for scalar add/sub is a reasonable enough assumption to make even for the generic cost model.

RKSimon added inline comments.Jan 23 2019, 3:27 AM
include/llvm/CodeGen/BasicTTIImpl.h
1278 ↗(On Diff #182509)

I've raised D57090 which will allow us to use overflow costs.

RKSimon updated this revision to Diff 183113.Jan 23 2019, 9:22 AM
RKSimon retitled this revision from [TTI] Add generic USUBSAT costs to [TTI] Add generic UADDSAT/USUBSAT and UADDO/USUBO costs.
RKSimon edited the summary of this revision. (Show Details)
RKSimon added reviewers: craig.topper, ABataev.

I intend to add the overflow costs support as a pre-commit.

nikic added inline comments.Jan 23 2019, 12:47 PM
test/Analysis/CostModel/X86/arith-usat.ll
313 ↗(On Diff #182509)

Unfortunately the comparison cost modeling improvements you implemented end up not applying here, because we don't have an instruction available :( But that doesn't look so simple to fix (would need to change the API to explicitly pass the predicate for that).

RKSimon marked an inline comment as done.Jan 23 2019, 1:23 PM
RKSimon added inline comments.
test/Analysis/CostModel/X86/arith-usat.ll
313 ↗(On Diff #182509)

Are you talking about PR40376 ? That work is still going on and its possibly a massive yak to shave. I don't see why that should stop us getting on with this patch.

nikic accepted this revision.Jan 23 2019, 2:14 PM

LGTM

test/Analysis/CostModel/X86/arith-usat.ll
313 ↗(On Diff #182509)

Sorry, didn't mean to imply that this is a blocker. I just initially thought that D57013 would already resolve this.

This revision is now accepted and ready to land.Jan 23 2019, 2:14 PM
This revision was automatically updated to reflect the committed changes.