This is an archive of the discontinued LLVM Phabricator instance.

[PartialInliner] Make PHIs free in cost computation.
ClosedPublic

Authored by fhahn on Nov 20 2018, 3:38 AM.

Details

Summary

InlineCost also treats them as free and the current implementation
can cause assertion failures if PHI nodes are moved outside the region
from entry BBs to the region.

It also updates the code to use the instructionsWithoutDebug iterator.

Diff Detail

Event Timeline

fhahn created this revision.Nov 20 2018, 3:38 AM
fhahn added a subscriber: huntergr.Nov 20 2018, 3:38 AM
davidxl added inline comments.Nov 26 2018, 10:08 AM
lib/Transforms/IPO/PartialInlining.cpp
334

Can you add documentation of IsEntryBB parameter (though it seems obvious)?

856

Since you are here, i think it is better to skip all 'nop' operations including (regardless wether it is in entry or not).

  1. debugintrinsic (already skipped)
  2. phi node
  3. nop casting
  4. functions calls that does not lower to actual calls (TTI.isLoweredToCall())
fhahn updated this revision to Diff 175442.Nov 27 2018, 3:16 AM
fhahn marked 2 inline comments as done.
fhahn retitled this revision from [PartialInliner] Skip PHIs in entry BB of outlined region. to [PartialInliner] Make PHIs free in cost computation..
fhahn edited the summary of this revision. (Show Details)

Thanks for having a look! Updated to treat PHI nodes as free.

fhahn added inline comments.Nov 27 2018, 5:20 AM
lib/Transforms/IPO/PartialInlining.cpp
856

Since you are here, i think it is better to skip all 'nop' operations including (regardless wether it is in entry or not).

debugintrinsic (already skipped)

updated to use iterator.

phi node
nop casting

Done by the switch now.

functions calls that does not lower to actual calls (TTI.isLoweredToCall())

We are using getCallsiteCost from InlineCost.cpp to get the cost of function calls. Do you think it is worth the special case this in PartialInliner.cpp? If so, I think it would be better as a separate commit.

davidxl accepted this revision.Nov 27 2018, 8:40 AM

There are more rooms to improve, but can be done independently.

LGTM

This revision is now accepted and ready to land.Nov 27 2018, 8:40 AM
This revision was automatically updated to reflect the committed changes.