This is an archive of the discontinued LLVM Phabricator instance.

Chang sign of LastCallToStaticBouns
ClosedPublic

Authored by Prazek on Aug 5 2016, 1:23 PM.

Details

Summary

I think it is much better this way.
When I firstly saw line:

Cost += InlineConstants::LastCallToStaticBonus;

I though that this is a bug, because everywhere where the cost is being reduced
it is usuing -=.

Diff Detail

Event Timeline

Prazek updated this revision to Diff 67006.Aug 5 2016, 1:23 PM
Prazek retitled this revision from to Changed sign of LastCallToStaticBouns.
Prazek updated this object.
Prazek added reviewers: eraman, tejohnson, mehdi_amini.
Prazek added a subscriber: llvm-commits.
mehdi_amini added inline comments.Aug 5 2016, 1:31 PM
include/llvm/Analysis/InlineCost.h
30–31

It made more sense to me before: now "Bonus" and "Penalty" are both positive.

lib/Analysis/InlineCost.cpp
1265

Here is also +=, I feel you're creating some inconsistency

eraman added inline comments.Aug 5 2016, 1:34 PM
lib/Analysis/InlineCost.cpp
1260

This is not the only use of this constant - it is used in Inliner.cpp also. It also feels a bit odd to subtract a "bonus", but I have no strong opinions.

Prazek added a comment.Aug 5 2016, 1:48 PM

I don't think it is inconsistent.
The name "Bonus" suggest that it should be substracted to lower the cost, the name Penality suggest that it should be added so the cost rise.

Prazek updated this revision to Diff 67012.Aug 5 2016, 1:49 PM

Update missing inst

Prazek marked an inline comment as done.Aug 5 2016, 1:51 PM
Prazek added inline comments.
lib/Analysis/InlineCost.cpp
1260

Thanks, I grepped only this file, and llvm tests also passed...

Prazek marked 2 inline comments as done.Aug 5 2016, 2:00 PM

So IMHO it is better to express increase of cost by add and decrease by substract, than mixing both.

Prazek added inline comments.Aug 5 2016, 2:02 PM
lib/Analysis/InlineCost.cpp
1265

But right now it is consistent with line 1248 and 1252

mehdi_amini edited edge metadata.Aug 9 2016, 11:59 AM

I'm not totally convinced, but if someone else thinks it is a good idea I won't oppose either.

Prazek retitled this revision from Changed sign of LastCallToStaticBouns to Chang sign of LastCallToStaticBouns.Aug 9 2016, 1:36 PM
Prazek edited edge metadata.
tejohnson edited edge metadata.Aug 9 2016, 4:44 PM

I agree with Piotr - to me it seems more intuitive to subtract a bonus from the "Cost" and to add the penalty. I have to admit I had the same confusion the first time I looked at this code, until I discovered that the Bonus was negative.

hans accepted this revision.Aug 9 2016, 5:11 PM
hans added a reviewer: hans.
hans added a subscriber: hans.

lgtm, seems like an obvious improvement to me.

This revision is now accepted and ready to land.Aug 9 2016, 5:11 PM
Prazek updated this revision to Diff 67602.Aug 10 2016, 2:22 PM
Prazek edited edge metadata.

rebase

This revision was automatically updated to reflect the committed changes.