This is an archive of the discontinued LLVM Phabricator instance.

[CodeGenPrep]No negative cost in the ExtLd promotion
ClosedPublic

Authored by junbuml on Jan 18 2017, 1:31 PM.

Details

Summary

This change prevent the signed value of cost from being negative as the value is passed as an unsigned argument.

Diff Detail

Event Timeline

junbuml created this revision.Jan 18 2017, 1:31 PM
junbuml set the repository for this revision to rL LLVM.Jan 18 2017, 1:34 PM
junbuml removed rL LLVM as the repository for this revision.
mcrosier accepted this revision.Jan 19 2017, 8:15 AM

LGTM.

This revision is now accepted and ready to land.Jan 19 2017, 8:15 AM
qcolombet edited edge metadata.Jan 25 2017, 11:04 AM

Hi,

I didn't look at the test case you're adding, but I guess it exposes the problem. Could you describe how the cost becomes negative?

I have a hard time picturing that and I want to be sure we are not missing something.
Indeed, at the beginning TotalCreatedInstsCost is CreatedInstsCost + NewCreatedInstsCost. Both operands are unsigned values.
Thus TotalCreatedInstsCost > 0.
Then TotalCreatedInstsCost -= ExtCost with ExtCost being either 0 or 1.
Thus, TotalCreatedInstsCost will get < 0 if it was TotalCreatedInstsCost == 0 before that operation and ExtCost is 1. Given TotalCreatedInstsCost == 0, the created instructions are all free.

First, I am not saying that's impossible, I want to be sure this is what we want.
Second, TotalCreatedInstsCost becomes a benefit and I wonder if we want to keep propagating it instead of ceiling it at 0.

Thanks,
-Quentin

Hi Quentin,

I believe the test case I added shows the case where TotalCreatedInstsCost becomes negative :

  • In the very first call of extLdPromotion(), CreatedInstsCost is 0, ExtCost is 1 for the sext (%r), and NewCreatedInstsCost after promotion is 0 because the cost of sext becomes free after moving it up before %shl2.
  • Therefore, TotalCreatedInstsCost -= ExtCost becomes negative.

I think propagating the negative value of TotalCreatedInstsCost is also possible, but I think ceiling it to 0 is simple and conservative enough to handle this corner case.

This patch is a small particle from D27853, which should be rebased based on this.

Thanks,
Jun

junbuml updated this revision to Diff 85979.Jan 26 2017, 3:54 PM

Thanks Quentin for the review. I added a FIXME about propagating negative values.
I will commit it if you don't have any further objection, and revisit it to see if propagating negative values is reasonable.

qcolombet accepted this revision.Jan 27 2017, 8:53 AM

Looks and sounds good to me!

Thanks,
Q

This revision was automatically updated to reflect the committed changes.