[PATCH] D110922: [LoopPeel] Peel loops with deoptimizing exits

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h:132
 
+/// Checks if the block or any of its children (or any of their children, etc)
+/// is "deoptimizing", i.e. either has a @llvm.experimental.deoptimize call
----------------
This comment doesn't correspond to what you really want. Imagine that a block has 2 successors, one being deoptimizing and another is just a regular block. It falls into category "any of its children is deoptimizing", but it's definitely not what you are looking for.

What I suggest is to rewrite it like "Check if we can prove that all paths starting from this block will converge to a block that is terminated by unreachable".


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:490
+    const BasicBlock *BB,
+    SmallVector<const BasicBlock *, 8> &DeoptimizingBlocks) {
+  const BasicBlock *Succ = BB;
----------------
What `DeoptimizingBlocks` is used for? To me it looks like a complete doubler of `VisitedBlocks`. Also no good reason for it to be a parameter.


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:493
+  // Remember visited blocks to avoid infinite loop
+  SmallVector<const BasicBlock *, 8> VisitedBlocks;
+  while (Succ) {
----------------
Use `SmallPtrSet`. `contains` checks in vector are expensive.


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:494
+  SmallVector<const BasicBlock *, 8> VisitedBlocks;
+  while (Succ) {
+    // Already proved this block is deoptimizing
----------------
Looks like it may be done much easier.
```
  while(BB && VisitedBlocks.insert(BB).second) {
    if ([terminates with unreachable](BB))
      return true;
     BB = BB->getSingleSuccessor();
   }
  return false;
```
Please check if it's what you meant here, and simplify the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110922