[PATCH] D101409: [SCEV] Check IDom with single predecessor on getPredecessorWithUniqueSuccessorForBB

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:9313
+      return {IDomPred, IDomBB};
+  }
+
----------------
I was going to suggest that we can generalize this by walking up the idom chain (this also removes the need to special case loops):

```
  while (true) {
    if (const BasicBlock *Pred = BB->getSinglePredecessor())
      return {Pred, BB};

    DomTreeNode *IDom = DT.getNode(BB)->getIDom();
    if (!IDom)
      break;

    BB = IDom->getBlock();
  }

  return {nullptr, nullptr};
```

Unfortunately, this has a bad compile-time impact: https://llvm-compile-time-tracker.com/compare.php?from=41849a91956755e15591240816d1d0c5ec402895&to=41241255419965bbfbc6d361058ea02449be9c50&stat=instructions ClamAV is up nearly 3%.

So I also tested your original patch: https://llvm-compile-time-tracker.com/compare.php?from=41849a91956755e15591240816d1d0c5ec402895&to=a012ed61e795b191cd7d114bfe972ce05c9f3d6b&stat=instructions This is better, but still bad, especially with little codegen impact.

isImpliedCond() is notoriously expensive, so compile-time is sensitive to the amount of guards we look at. I think if we want to do any extensions here, we need to also aggressively limit the number of edges for which we take conditions into account.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101409/new/

https://reviews.llvm.org/D101409