[PGO] Early detection regarding whether pgo counter promotion is possible

[chris.chrulski]

Summary

This fixes a problem with the current behavior when assertions are enabled.
A loop that exits to a catchswitch instruction is skipped for the counter
promotion, however this check was being done after the PGOCounterPromoter
tried to collect an insertion point for the exit block. A call to
getFirstInsertionPt() on a block that begins with a catchswitch instruction
triggers an assertion. This change performs a check whether the counter
promotion is possible prior to collecting the ExitBlocks and InsertPts.

[davidxl]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

362

Why this change (changing ExitingBlocks to ExitBlocks)?

[chris.chrulski]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

362

Sorry, that was a mistake on my part. Thanks for catching that. I misread that one was setting 'exiting' vs' exit' in the call. will update patch.

[chris.chrulski]

Fix for bug in initial revision.

[davidxl]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

328

You can pass LoopExitBlocks reference as a parameter to this function so that the call to getExitBlocks can be shared with the caller of it.

[chris.chrulski]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

328

ok. will do.

[chris.chrulski]

Updated interface for isPromotionPossible routine to take in LoopExitBlocks.

[davidxl]

lgtm

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

 class PGOCounterPromoter {
 public:
   PGOCounterPromoter(
       DenseMap<Loop *, SmallVector<LoadStorePair, 8>> &LoopToCands,
       Loop &CurLoop, LoopInfo &LI, BlockFrequencyInfo *BFI)
       : LoopToCandidates(LoopToCands), ExitBlocks(), InsertPts(), L(CurLoop),
         LI(LI), BFI(BFI) {
 
+    // Skip collection of ExitBlocks and InsertPts for loops that will not be
+    // able to have counters promoted.
     SmallVector<BasicBlock *, 8> LoopExitBlocks;
     SmallPtrSet<BasicBlock *, 8> BlockSet;
+
     L.getExitBlocks(LoopExitBlocks);
+    if (!isPromotionPossible(&L, LoopExitBlocks))
+      return;
 
     for (BasicBlock *ExitBlock : LoopExitBlocks) {
       if (BlockSet.insert(ExitBlock).second) {
         ExitBlocks.push_back(ExitBlock);
         InsertPts.push_back(&*ExitBlock->getFirstInsertionPt());
       }
     }
   }
bool allowSpeculativeCounterPromotion(Loop *LP) {
     // Not considierered speculative.
     if (ExitingBlocks.size() == 1)
       return true;
     if (ExitingBlocks.size() > SpeculativeCounterPromotionMaxExiting)
       return false;
     return true;
   }
 
-  // Returns the max number of Counter Promotions for LP.
-  unsigned getMaxNumOfPromotionsInLoop(Loop *LP) {
+  // Check whether the loop satisfies the basic conditions needed to perform
+  // Counter Promotions.
+  bool isPromotionPossible(Loop *LP,
+                           const SmallVectorImpl<BasicBlock *> &LoopExitBlocks) {
     // We can't insert into a catchswitch.
-    SmallVector<BasicBlock *, 8> LoopExitBlocks;
-    LP->getExitBlocks(LoopExitBlocks);
     if (llvm::any_of(LoopExitBlocks, [](BasicBlock *Exit) {
           return isa<CatchSwitchInst>(Exit->getTerminator());
         }))
-      return 0;
+      return false;
 
     if (!LP->hasDedicatedExits())
-      return 0;
+      return false;
 
     BasicBlock *PH = LP->getLoopPreheader();
     if (!PH)
+      return false;
+
+    return true;
+  }
+
+  // Returns the max number of Counter Promotions for LP.
+  unsigned getMaxNumOfPromotionsInLoop(Loop *LP) {
+    SmallVector<BasicBlock *, 8> LoopExitBlocks;
+    LP->getExitBlocks(LoopExitBlocks);
+    if (!isPromotionPossible(LP, LoopExitBlocks))
       return 0;
 
     SmallVector<BasicBlock *, 8> ExitingBlocks;
     LP->getExitingBlocks(ExitingBlocks);
 
     // If BFI is set, we do more aggressive promotions based on BFI.
     if (BFI)
       return (unsigned)-1;
 
     // Not considierered speculative.
     if (ExitingBlocks.size() == 1)
       return MaxNumOfPromotionsPerLoop;
 
     if (ExitingBlocks.size() > SpeculativeCounterPromotionMaxExiting)
       return 0;
 
     // Whether the target block is in a loop does not matter:
     if (SpeculativeCounterPromotionToLoop)
       return MaxNumOfPromotionsPerLoop;
 

llvm/test/Transforms/PGOProfile/counter_promo_exit_catchswitch.ll

+; Test that instrumentation counter promotion for loops does not fail during
+; compilation for loops that exit to a catchswitch block. In this case, counters
+; do not get promoted out of the loop body.
+
+; RUN: opt < %s -pgo-instr-gen -instrprof -do-counter-promotion=true -S | FileCheck %s
+; RUN: opt < %s -passes=pgo-instr-gen,instrprof -do-counter-promotion=true -S | FileCheck %s
+
+; Source used to create test:
+;
+; extern void may_throw(int);
+; char buffer[200];
+; void run(int count) {
+;   try {
+;    for (int i = 0; i < count; ++i) {
+;      if (buffer[i] == 0)
+;        break;
+;      may_throw(i);
+;    }
+;  }
+;  catch (...) {
+;     throw;
+;  }
+;}
+
+%eh.ThrowInfo = type { i32, i32, i32, i32 }
+
+@"?buffer@@3PADA" = dso_local local_unnamed_addr global [200 x i8] zeroinitializer, align 16
+define dso_local void @"?run@@YAXH@Z"(i32 %count) local_unnamed_addr personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.inc, %entry
+  %i.0 = phi i32 [ 0, %entry ], [ %inc, %for.inc ]
+  %cmp = icmp slt i32 %i.0, %count
+  br i1 %cmp, label %for.body, label %cleanup
+
+for.body:                                         ; preds = %for.cond
+; CHECK: for.body:
+; CHECK: %pgocount1 = load i64, i64* getelementptr inbounds ([3 x i64], [3 x i64]* @"__profc_?run@@YAXH@Z", i64 0, i64 0)
+; CHECK: %1 = add i64 %pgocount1, 1
+; CHECK: store i64 %1, i64* getelementptr inbounds ([3 x i64], [3 x i64]* @"__profc_?run@@YAXH@Z", i64 0, i64 0)
+  %idxprom = zext i32 %i.0 to i64
+  %arrayidx = getelementptr inbounds [200 x i8], [200 x i8]* @"?buffer@@3PADA", i64 0, i64 %idxprom
+  %0 = load i8, i8* %arrayidx, align 1
+  %cmp1 = icmp eq i8 %0, 0
+  br i1 %cmp1, label %cleanup, label %if.end
+
+if.end:                                           ; preds = %for.body
+  invoke void @"?may_throw@@YAXH@Z"(i32 %i.0)
+          to label %for.inc unwind label %catch.dispatch
+
+for.inc:                                          ; preds = %if.end
+; CHECK: for.inc:
+; CHECK: %pgocount2 = load i64, i64* getelementptr inbounds ([3 x i64], [3 x i64]* @"__profc_?run@@YAXH@Z", i64 0, i64 1)
+; CHECK: %3 = add i64 %pgocount2, 1
+; CHECK: store i64 %3, i64* getelementptr inbounds ([3 x i64], [3 x i64]* @"__profc_?run@@YAXH@Z", i64 0, i64 1)
+  %inc = add nuw nsw i32 %i.0, 1
+  br label %for.cond
+
+cleanup:                                          ; preds = %for.body, %for.cond
+  ret void
+
+catch.dispatch:                                   ; preds = %if.end
+  %1 = catchswitch within none [label %catch] unwind to caller
+
+catch:                                            ; preds = %catch.dispatch
+  %2 = catchpad within %1 [i8* null, i32 64, i8* null]
+  call void @_CxxThrowException(i8* null, %eh.ThrowInfo* null) #2 [ "funclet"(token %2) ]
+  unreachable
+}
+declare dso_local void @"?may_throw@@YAXH@Z"(i32)
+declare dso_local void @_CxxThrowException(i8*, %eh.ThrowInfo*)
+declare dso_local i32 @__CxxFrameHandler3(...)