This is an archive of the discontinued LLVM Phabricator instance.

[TTI] Make the cost APIs in TargetTransformInfo consistently use 'int' rather than 'unsigned' for their costs.
ClosedPublic

Authored by chandlerc on Aug 3 2015, 4:55 PM.

Details

Summary

For something like costs in particular there is a natural "negative"
value, that of savings or saved cost. As a consequence, there is a lot
of code that subtracts or creates negative values based on cost, all of
which is prone to awkwardness or bugs when dealing with an unsigned
type. Similarly, we *never* want these values to wrap, as that would
cause Very Bad code generation (likely percieved as an infinite loop as
we try to emit over 2^32 instructions or some such insanity).

All around 'int' seems a much better fit for these basic metrics.

No functional change intended.

Diff Detail

Event Timeline

chandlerc updated this revision to Diff 31286.Aug 3 2015, 4:55 PM
chandlerc retitled this revision from to [TTI] Make the cost APIs in TargetTransformInfo consistently use 'int' rather than 'unsigned' for their costs..
chandlerc updated this object.
chandlerc added a reviewer: echristo.
chandlerc added a subscriber: llvm-commits.
sanjoy added a subscriber: sanjoy.Aug 3 2015, 5:03 PM

I have two comments:

  • I'd be scared of existing code that returns -1 to mean "very expensive". Now that same value means "cheaper than free". I think these should be fixed, but is there a way to automatically flush these out?
  • Do you intend to add callbacks that actually return negative costs? Or is this meant to make intermediate computation more ergonomic?

I have two comments:

  • I'd be scared of existing code that returns -1 to mean "very expensive". Now that same value means "cheaper than free". I think these should be fixed, but is there a way to automatically flush these out?

Probably, I think I can add some asserts to check for this. But I've also read through most of this code and don't see anyone doing that. Still, better to check.

  • Do you intend to add callbacks that actually return negative costs? Or is this meant to make intermediate computation more ergonomic?

The latter. In particular, I have seen several cases where we might reasonably wrap up computation or aggregated calls to this API and return a potentially negative number. With aggregation this becomes more more believable. And it is quite awkward to force such code to switch types suddenly.

chandlerc updated this revision to Diff 31292.Aug 3 2015, 6:38 PM

Updated after running clang-format over all the code and adding asserts to
address Sanjoy's concerns.

I have two comments:

  • I'd be scared of existing code that returns -1 to mean "very expensive". Now that same value means "cheaper than free". I think these should be fixed, but is there a way to automatically flush these out?

Probably, I think I can add some asserts to check for this. But I've also read through most of this code and don't see anyone doing that. Still, better to check.

Done, now with asserts that we don't misuse -1 inside of TTI.

Ping, should be a simple patch...

mcrosier accepted this revision.Aug 5 2015, 11:03 AM
mcrosier added a reviewer: mcrosier.
mcrosier added a subscriber: mcrosier.

My finger now hurts from scrolling... LGTM.

This revision is now accepted and ready to land.Aug 5 2015, 11:03 AM

My finger now hurts from scrolling... LGTM.

Awesome, thanks!

This revision was automatically updated to reflect the committed changes.