[LoopBase] Strengthen isLoopExiting by requiring that BB must be inside the loop.

[fhahn]

Summary

Currently isLoopExiting returns true for BBs that are not part of the
loop. To avoid hiding subtle bugs, this patch adds an assertion to make
sure the passed BB is inside the loop. Alternatively, we could return
false for BBs outside the loop, but I think it makes sense to restrict
it to BBs inside a loop.

Just one use in AMDGPUTTIImpl needed updating.

[fhahn]

Adding @arsenm & @nhaehnle for the AMDGPU part.

[efriedma]

I agree the change to the API makes sense.

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

122

This seems substantially different from the original logic. Probably it should be if (!L->contains(Succ0) || L->isLoopExiting(Succ0) || [...]... which would be effectively the same as the original logic in most cases. Given a loop in LoopSimplify form, the only case I can think of where it wouldn't be the same is if Succ0 or Succ1 has no successors (ends in a "ret" or "unreachable").

[fhahn]

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

122

I originally thought the original check was intended to check for actual exiting blocks, but I can change it to check for exiting blocks or blocks outside the loop, if that is the intended behavior.

[reames]

LGTM to the LoopInfo change only. The other portion should probably be done in a separate change as the set of reviewers are likely to be basically distinct. (I don't care which order.)

[fhahn]

Split off AMDGPU part to D63980

[fhahn]

The AMDGPU fix was committed, so this should be ready to go in :) @reames, it would be great if you could accept it on Phab

[reames]

In D63952#1568379, @fhahn wrote:

The AMDGPU fix was committed, so this should be ready to go in :) @reames, it would be great if you could accept it on Phab

Did so, but this is not required. Once an LGTM has been given, a patch is clear to land whether accepted in phab or not.

llvm/include/llvm/Analysis/LoopInfo.h

 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
     return IsInvalid;
 #else
     return false;
 #endif
   }
 
   /// True if terminator in the block can branch to another block that is
-  /// outside of the current loop.
+  /// outside of the current loop. \p BB must be inside the loop.
   bool isLoopExiting(const BlockT *BB) const {
     assert(!isInvalid() && "Loop not in a valid state!");
+    assert(contains(BB) && "Exiting block must be part of the loop");
     for (const auto &Succ : children<const BlockT *>(BB)) {
       if (!contains(Succ))
         return true;
     }
     return false;
   }
 
   /// Returns true if \p BB is a loop-latch.

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

for (const BasicBlock *BB : L->getBlocks()) {
     for (const Instruction &I : *BB) {
       // Unroll a loop which contains an "if" statement whose condition
       // defined by a PHI belonging to the loop. This may help to eliminate
       // if region and potentially even PHI itself, saving on both divergence
       // and registers used for the PHI.
       // Add a small bonus for each of such "if" statements.
       if (const BranchInst *Br = dyn_cast<BranchInst>(&I)) {
         if (UP.Threshold < MaxBoost && Br->isConditional()) {
-          if (L->isLoopExiting(Br->getSuccessor(0)) ||
-              L->isLoopExiting(Br->getSuccessor(1)))
+          BasicBlock *Succ0 = Br->getSuccessor(0);
+          BasicBlock *Succ1 = Br->getSuccessor(1);
+          if ((L->contains(Succ0) && L->isLoopExiting(Succ0)) ||
+              (L->contains(Succ1) && L->isLoopExiting(Succ1)))
             continue;
           if (dependsOnLocalPhi(L, Br->getCondition())) {
             UP.Threshold += UnrollThresholdIf;
             LLVM_DEBUG(dbgs() << "Set unroll threshold " << UP.Threshold
                               << " for loop:\n"
                               << *L << " due to " << *Br << '\n');
             if (UP.Threshold >= MaxBoost)
               return;