Memory intrinsic value profile optimization: Avoid divide by 0

[tejohnson]

Summary

Skip memops if the total value profiled count is 0, we can't correctly
scale up the counts and there is no point anyway.

[davidxl]

lib/Transforms/Instrumentation/IndirectCallPromotion.cpp

877 ↗

(On Diff #97018)

Or change it to

if (TotalCount < MemOpCountThreshold)
return false;

and move this check before the actual count check.

[tejohnson]

lib/Transforms/Instrumentation/IndirectCallPromotion.cpp

877 ↗

(On Diff #97018)

I could move this up, but note that the counts will be scaled up by ActualCount/SavedTotalCount (where SavedTotalCount == TotalCount at this point). So it is possible for TotalCount < MemOpCountThreshold but not ActualCount, and not the scaled counts. So that check seems overly conservative (and will have an effect other than just preventing 0 divides).

[davidxl]

lib/Transforms/Instrumentation/IndirectCallPromotion.cpp

877 ↗

(On Diff #97018)

The scale is actually to scale down for most the cases -- this is because BB's count will be updated after inlining, while value profiling total count will be updated. In other words, it won't cause conservative behavior for hot sites.

[tejohnson]

lib/Transforms/Instrumentation/IndirectCallPromotion.cpp

877 ↗

(On Diff #97018)

I checked in one of our large apps, and there are over 44K memory intrinsics where we scale up the counts. So at the least, I would like to consider that change separately from this bugfix.

[davidxl]

lgtm

llvm/trunk/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp

bool MemOPSizeOpt::perform(MemIntrinsic *MI) {
   DEBUG(dbgs() << "Read one memory intrinsic profile with count " << ActualCount
                << "\n");
   DEBUG(
       for (auto &VD
            : VDs) { dbgs() << "  (" << VD.Value << "," << VD.Count << ")\n"; });
 
   if (ActualCount < MemOPCountThreshold)
     return false;
+  // Skip if the total value profiled count is 0, in which case we can't
+  // scale up the counts properly (and there is no profitable transformation).
+  if (TotalCount == 0)
+    return false;
 
   TotalCount = ActualCount;
   if (MemOPScaleCount)
     DEBUG(dbgs() << "Scale counts: numerator = " << ActualCount
                  << " denominator = " << SavedTotalCount << "\n");
 
   // Keeping track of the count of the default case:
   uint64_t RemainCount = TotalCount;

llvm/trunk/test/Transforms/PGOProfile/memop_size_opt_zero.ll

+; Test to ensure the pgo memop optimization pass doesn't try to scale
+; up a value profile with a 0 count, which would lead to divide by 0.
+; RUN: opt < %s -passes=pgo-memop-opt -pgo-memop-count-threshold=1 -S | FileCheck %s --check-prefix=MEMOP_OPT
+; RUN: opt < %s -pgo-memop-opt -pgo-memop-count-threshold=1 -S | FileCheck %s --check-prefix=MEMOP_OPT
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @foo(i8* %dst, i8* %src, i64 %conv) !prof !0 {
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src, i64 %conv, i32 1, i1 false), !prof !1
+  ret void
+}
+
+; MEMOP_OPT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src, i64 %conv, i32 1, i1 false), !prof !1
+
+!0 = !{!"function_entry_count", i64 1}
+!1 = !{!"VP", i32 1, i64 0, i64 1, i64 0, i64 2, i64 0, i64 3, i64 0, i64 9, i64 0, i64 4, i64 0, i64 5, i64 0, i64 6, i64 0, i64 7, i64 0, i64 8, i64 0}
+
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i32, i1)