[PATCH] D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

In https://reviews.llvm.org/D44177#1040497, @efriedma wrote:

> Could this lead to problems when jump threading erases an edge?


Hi @efriedma , it is possible there are still problems inside JumpThreading related to unreachable block chains. However, this patch wouldn't make that problem any better (or worse) than how JumpThreading was before.  Here's the high-level algorithm of JumpThreading before this change:

  removeUnreachableBlocks(F);
  while Changed:
    for BB in F:
      ProcessBlock(BB);  // Can modify Changed
      // Try to delete BB (can modify Changed)
      // Try to merge BB with its successor (can modify Changed) 

Previously removeUnreachableBlocks(F) removed unreachable blocks (and altered the reachable blocks) before starting its main `while Changed` loop. There are several places in JumpThreading where blocks were deleted or altered that didn't necessarily lead to threadable edges: `DeleteDeadBlock`, `TryToSimplifyUncondBranchFromEmptyBlock`, `MergeBasicBlockIntoOnlyPred`,  and `ConstantFoldTerminator`.  Any one of these calls could lead to unreachable regions being generated and they weren't being tracked before.

This patch works almost identical to the old algorithm:

  // Generate Unreachable Blocks Set
  while Changed:
    if BB in Unreachable:
      continue;
    for BB in F:
      ProcessBlock(BB);  // Can modify Changed
      // Try to delete BB (can modify Changed)
      // Try to merge BB with its successor (can modify Changed) 

The big difference here is instead of actually removing the blocks we simply ignore them. But in both cases they are identified once before the main `while Changed` loop starts.

The only way I know of preventing any wasted work done on unreachable regions is to check if BB is to maintain a set of blocks that are reachable from entry. We'd need to  verify this state with calls to DT on almost every iteration inside ProcessBlock. It's not practical as it greatly lengthens compile time. For large functions like those found in `sqlite3.c` I was seeing a slowdown of 20% or more to track the 3,000+ blocks in the main REPL function when I initially tried this approach.

If you have ideas to better test IR coverage for JumpThreading I'd be very interested in hearing about them.


Repository:
  rL LLVM

https://reviews.llvm.org/D44177