This is an archive of the discontinued LLVM Phabricator instance.

[PartialInliner]: Handle code regions in a switch stmt cases
ClosedPublic

Authored by etiotto on Oct 21 2020, 2:26 PM.

Details

Summary

Currently the following C example fails to be partially inlined. The issue is that computeOutliningColdRegionsInfo checks for candidate blocks that have a single entry, but blocks that dominate only themselves fail the check. Example (this is also added as a LLVM IR test):

int callee(int c1, int c2) {
  int rc = 0;

  switch(c1) {
  case 0: // cold
    rc = 1;
    break;
  case 1: // warm
    rc = 2;
    break;
  case 2: // cold
    rc = 4;
    break;
  default: //hot
    rc = c2;
  }

  return rc;
}

int caller(int c1) {
  int rc = callee(c1, c1);
  return rc;
}

With this patch the code in the 2 cold switch cases will be outlined and the remaining function inlined in its caller.

Diff Detail

Event Timeline

etiotto created this revision.Oct 21 2020, 2:26 PM
etiotto requested review of this revision.Oct 21 2020, 2:26 PM
Whitney added inline comments.Oct 21 2020, 4:16 PM
llvm/lib/Transforms/IPO/PartialInlining.cpp
418

This change allows the single basic block to have multiple predecessor, which is what this function want to avoid.
How about changing BlockList.size() > 1 && to BlockList.size() >= 1 &&?

etiotto updated this revision to Diff 299954.Oct 22 2020, 6:52 AM
etiotto marked an inline comment as done.

Address comments

etiotto edited the summary of this revision. (Show Details)Oct 22 2020, 6:53 AM
fhahn added a subscriber: fhahn.Oct 22 2020, 8:12 AM
fhahn added inline comments.
llvm/test/Transforms/PartialInlining/switch_stmt.ll
3

run line for old PM, as it is still the default?

fhahn added a reviewer: vsk.Oct 22 2020, 8:38 AM
etiotto updated this revision to Diff 300002.Oct 22 2020, 8:59 AM
etiotto added reviewers: fhahn, gyiu.

Address comments.

etiotto updated this revision to Diff 300078.Oct 22 2020, 12:37 PM
etiotto marked an inline comment as done.

Add LLVM_DEBUG stmts.

etiotto updated this revision to Diff 300678.Oct 26 2020, 8:06 AM

Fix formatting.

LGTM. I will approve tomorrow if no one object..

fhahn added inline comments.Oct 30 2020, 9:49 AM
llvm/lib/Transforms/IPO/PartialInlining.cpp
509

It seems like it would be simpler to get rid of the isSingleEntry lambda all together and inline the checks.

The check BlockList.size() >= 1 in particular seems redundant. The only case where DominateVector may be empty is if unreachable I think. SI is a successor of a node we found during DFS traversal from the entry block, so all SIs should be reachable? So could this just be something like

  assert(!DominateVector.empty() && "SI should be reachable and have at least itself as descendant");
  // We can only outline single entry regions (for now).
  if (!DominateVector.front()->hasNPredecessors(1)) {
...
511

nit: isSingleEntry checks the predecessors in the dom tree, not the direct predecessors of the node. Might be good to clarify

533

I am not sure why we say ABORT: here but not at the other, similar continues?

etiotto marked 3 inline comments as done.Oct 30 2020, 11:42 AM
etiotto added inline comments.
llvm/lib/Transforms/IPO/PartialInlining.cpp
533

I'll add similar debug messages in the other 'continue' conditions

etiotto updated this revision to Diff 301975.Oct 30 2020, 11:44 AM
etiotto marked an inline comment as done.

Address code review comments.

fhahn accepted this revision.Oct 30 2020, 3:03 PM

Lgtm, thanks

This revision is now accepted and ready to land.Oct 30 2020, 3:03 PM
This revision was landed with ongoing or failed builds.Nov 2 2020, 11:33 AM
This revision was automatically updated to reflect the committed changes.