Page MenuHomePhabricator

[SCEV] isHighCostExpansionHelper(): use correct TTI hooks
ClosedPublic

Authored by lebedev.ri on Mar 10 2020, 5:04 AM.

Details

Summary

Cost modelling strikes again.
In PR44668 https://bugs.llvm.org/show_bug.cgi?id=44668 patch series,
i've made the same mistake of always using generic getOperationCost()
that i missed in reviewing D73480/D74495 which was later fixed
in 62dd44d76da9aa596fb199bda8b1e8768bb41033.

We should be using more specific hooks instead - getCastInstrCost(),
getArithmeticInstrCost(), getCmpSelInstrCost().

Evidently, this does not have an effect on the existing testcases,
with unchanged default cost budget. But if it *does* have an effect
on some target, we'll have to segregate tests that use this function
per-target, much like we already do with other TTI-aware transform tests.

There's also an issue that @samparker has brought up in post-commit-review:

Hi,
Did you get performance numbers for these patches? We track the performance
of our (Arm) open source DSP library and the cost model fixes were generally
a notable improvement, so many thanks for that! But the final patch
for rewriting exit values has generally been bad, especially considering
the gains from the modelling improvements. I need to look into it further,
but on my current test case I'm seeing +30% increase in stack accesses
with a similar decrease in performance.
I'm just wondering if you observed any negative effects yourself?

I don't know if this addresses that, or we need D66450 for that.

Diff Detail

Event Timeline

lebedev.ri created this revision.Mar 10 2020, 5:04 AM
reames accepted this revision.Mar 10 2020, 5:44 PM

LGTM.

Though, I'll comment that this interface is broken. If the generically named function doesn't do the same thing as the specifically named version given the same arguments, that's a bug in the API design. The only one of these which is defensible is the select changes. The LGTM is because this is a generic problem, and I don't want to block progress in this specific area on the generic fix.

This revision is now accepted and ready to land.Mar 10 2020, 5:44 PM

Agreed that it's broken, but it looks like SCEV expander is the only place that this API is/was used. Can we now remove getOperationCost?

Thank you for the review.

This revision was automatically updated to reflect the committed changes.

Agreed that it's broken, but it looks like SCEV expander is the only place that this API is/was used. Can we now remove getOperationCost?

If the code is dead, sure. That's a great way to fix broken code. :)

Agreed that it's broken, but it looks like SCEV expander is the only place that this API is/was used. Can we now remove getOperationCost?

If the code is dead, sure. That's a great way to fix broken code. :)

+1. If it's really not used let's drop it. It was mis-used twice already in just last month or so.

Then I'll get a patch together, hopefully then we can transition TTI away from the under-implemented parts of the API.