[PGO] Fix two issues in PGOMemOPSizeOpt.

[hjyamauchi]

Summary

1. PGOMemOPSizeOpt grabs only the first, up to five (by default) entries from

the value profile metadata and preserves the remaining entries for the fallback
memop call site. If there are more than five entries, the rest of the entries
would get dropped. This is fine for PGOMemOPSizeOpt itself as it only promotes
up to 3 (by default) values, but potentially not for other downstream passes
that may use the value profile metadata.

2. PGOMemOPSizeOpt originally assumed that only values 0 through 8 are kept

track of. When the range buckets were introduced, it was changed to skip the
range buckets, but since it does not grab all entries (only five), if some range
buckets exist in the first five entries, it could potentially cause fewer
promotion opportunities (eg. if 4 out of 5 were range buckets, it may be able to
promote up to one non-range bucket, as opposed to 3.) Also, combined with 1, it
means that wrong entries may be preserved, as it didn't correctly keep track of
which were entries were skipped.

To fix this, PGOMemOPSizeOpt now grabs all the entries (up to the maximum number
of value profile buckets), keeps track of which entries were skipped, and
preserves all the remaining entries.

[davidxl]

Perhaps add a test case showing that up to 3 ranges can be promoted?

[hjyamauchi]

In D97592#2593258, @davidxl wrote:

Perhaps add a test case showing that up to 3 ranges can be promoted?

Do you mean one in general or for 2 (specially this part "if 4 out of 5 were range buckets, it may be able to
promote up to one non-range bucket, as opposed to 3.")?

[hjyamauchi]

Address comment.

[hjyamauchi]

Okay added a new test.

[davidxl]

compiler-rt/include/profile/InstrProfData.inc

864

should we define a macro for it?

[hjyamauchi]

Address comment.

[davidxl]

lgtm

compiler-rt/include/profile/InstrProfData.inc

  *   [257, 511]
  *   [512, 512]
  *   [513, UINT64_MAX]
  *
  * Each range has a 'representative value' which is the lower end value of the
  * range and used to store in the runtime profile data records and the VP
  * metadata. For example, it's 2 for [2, 2] and 64 for [65, 127].
  */
+#define INSTR_PROF_NUM_BUCKETS 22
 
 /*
  * Clz and Popcount. This code was copied from
  * compiler-rt/lib/fuzzer/{FuzzerBuiltins.h,FuzzerBuiltinsMsvc.h} and
  * llvm/include/llvm/Support/MathExtras.h.
  */
 #if defined(_MSC_VER) && !defined(__clang__)
 

 INSTR_PROF_VISIBILITY INSTR_PROF_INLINE
 int InstProfPopcountll(unsigned long long X) { return __builtin_popcountll(X); }
 
 #endif  /* defined(_MSC_VER) && !defined(__clang__) */
 
 /* Map an (observed) memop size value to the representative value of its range.
  * For example, 5 -> 5, 22 -> 17, 99 -> 65, 256 -> 256, 1001 -> 513. */
 INSTR_PROF_VISIBILITY INSTR_PROF_INLINE uint64_t
 InstrProfGetRangeRepValue(uint64_t Value) {
   if (Value <= 8)
     // The first ranges are individually tracked. Use the value as is.
     return Value;
   else if (Value >= 513)
     // The last range is mapped to its lowest value.
     return 513;
   else if (InstProfPopcountll(Value) == 1)
     // If it's a power of two, use it as is.

llvm/include/llvm/ProfileData/InstrProfData.inc

  *   [257, 511]
  *   [512, 512]
  *   [513, UINT64_MAX]
  *
  * Each range has a 'representative value' which is the lower end value of the
  * range and used to store in the runtime profile data records and the VP
  * metadata. For example, it's 2 for [2, 2] and 64 for [65, 127].
  */
+#define INSTR_PROF_NUM_BUCKETS 22
 
 /*
  * Clz and Popcount. This code was copied from
  * compiler-rt/lib/fuzzer/{FuzzerBuiltins.h,FuzzerBuiltinsMsvc.h} and
  * llvm/include/llvm/Support/MathExtras.h.
  */
 #if defined(_MSC_VER) && !defined(__clang__)
 

llvm/lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp

 
 class MemOPSizeOpt : public InstVisitor<MemOPSizeOpt> {
 public:
   MemOPSizeOpt(Function &Func, BlockFrequencyInfo &BFI,
                OptimizationRemarkEmitter &ORE, DominatorTree *DT,
                TargetLibraryInfo &TLI)
       : Func(Func), BFI(BFI), ORE(ORE), DT(DT), TLI(TLI), Changed(false) {
     ValueDataArray =
-        std::make_unique<InstrProfValueData[]>(MemOPMaxVersion + 2);
+        std::make_unique<InstrProfValueData[]>(INSTR_PROF_NUM_BUCKETS);
   }
   bool isChanged() const { return Changed; }
   void perform() {
     WorkList.clear();
     visit(Func);
 
     for (auto &MO : WorkList) {
       ++NumOfPGOMemOPAnnotate;

 
 bool MemOPSizeOpt::perform(MemOp MO) {
   assert(MO.I);
   if (MO.isMemmove())
     return false;
   if (!MemOPOptMemcmpBcmp && (MO.isMemcmp(TLI) || MO.isBcmp(TLI)))
     return false;
 
-  uint32_t NumVals, MaxNumPromotions = MemOPMaxVersion + 2;
+  uint32_t NumVals, MaxNumVals = INSTR_PROF_NUM_BUCKETS;
   uint64_t TotalCount;
-  if (!getValueProfDataFromInst(*MO.I, IPVK_MemOPSize, MaxNumPromotions,
+  if (!getValueProfDataFromInst(*MO.I, IPVK_MemOPSize, MaxNumVals,
                                 ValueDataArray.get(), NumVals, TotalCount))
     return false;
 
   uint64_t ActualCount = TotalCount;
   uint64_t SavedTotalCount = TotalCount;
   if (MemOPScaleCount) {
     auto BBEdgeCount = BFI.getBlockProfileCount(MO.I->getParent());
     if (!BBEdgeCount)
bool MemOPSizeOpt::perform(MemOp MO) {
   uint64_t SavedRemainCount = SavedTotalCount;
   SmallVector<uint64_t, 16> SizeIds;
   SmallVector<uint64_t, 16> CaseCounts;
   uint64_t MaxCount = 0;
   unsigned Version = 0;
   int64_t LastV = -1;
   // Default case is in the front -- save the slot here.
   CaseCounts.push_back(0);
-  for (auto &VD : VDs) {
+  SmallVector<InstrProfValueData, 24> RemainingVDs;
+  for (auto I = VDs.begin(), E = VDs.end(); I != E; ++I) {
+    auto &VD = *I;
     int64_t V = VD.Value;
     uint64_t C = VD.Count;
     if (MemOPScaleCount)
       C = getScaledCount(C, ActualCount, SavedTotalCount);
 
-    if (!InstrProfIsSingleValRange(V) || V > MemOpMaxOptSize)
+    if (!InstrProfIsSingleValRange(V) || V > MemOpMaxOptSize) {
+      RemainingVDs.push_back(VD);
       continue;
+    }
 
     // ValueCounts are sorted on the count. Break at the first un-profitable
     // value.
-    if (!isProfitable(C, RemainCount))
+    if (!isProfitable(C, RemainCount)) {
+      RemainingVDs.insert(RemainingVDs.end(), I, E);
       break;
+    }
 
     if (V == LastV) {
       LLVM_DEBUG(dbgs() << "Invalid Profile Data in Function " << Func.getName()
                         << ": Two consecutive, identical values in MemOp value"
                            "counts.\n");
       return false;
     }
 
     LastV = V;
 
     SizeIds.push_back(V);
     CaseCounts.push_back(C);
     if (C > MaxCount)
       MaxCount = C;
 
     assert(RemainCount >= C);
     RemainCount -= C;
     assert(SavedRemainCount >= VD.Count);
     SavedRemainCount -= VD.Count;
 
-    if (++Version > MemOPMaxVersion && MemOPMaxVersion != 0)
+    if (++Version >= MemOPMaxVersion && MemOPMaxVersion != 0) {
+      RemainingVDs.insert(RemainingVDs.end(), I + 1, E);
       break;
     }
+  }
 
   if (Version == 0)
     return false;
 
   CaseCounts[0] = RemainCount;
   if (RemainCount > MaxCount)
     MaxCount = RemainCount;
 
if (!MemOpTy->isVoidTy()) {
     PHI = IRBM.CreatePHI(MemOpTy, SizeIds.size() + 1, "MemOP.RVMerge");
     MO.I->replaceAllUsesWith(PHI);
     PHI->addIncoming(MO.I, DefaultBB);
   }
 
   // Clear the value profile data.
   MO.I->setMetadata(LLVMContext::MD_prof, nullptr);
   // If all promoted, we don't need the MD.prof metadata.
-  if (SavedRemainCount > 0 || Version != NumVals)
+  if (SavedRemainCount > 0 || Version != NumVals) {
     // Otherwise we need update with the un-promoted records back.
-    annotateValueSite(*Func.getParent(), *MO.I, VDs.slice(Version),
-                      SavedRemainCount, IPVK_MemOPSize, NumVals);
+    ArrayRef<InstrProfValueData> RemVDs(RemainingVDs);
+    annotateValueSite(*Func.getParent(), *MO.I, RemVDs, SavedRemainCount,
+                      IPVK_MemOPSize, NumVals);
+  }
 
   LLVM_DEBUG(dbgs() << "\n\n== Basic Block After==\n");
 
   std::vector<DominatorTree::UpdateType> Updates;
   if (DT)
     Updates.reserve(2 * SizeIds.size());
 
   for (uint64_t SizeId : SizeIds) {

llvm/test/Transforms/PGOProfile/memop_size_opt.ll

for.inc4:
   br label %for.cond
 
 for.end6:
   ret void
 }
 
 ; MEMOP_OPT: [[SWITCH_BW]] = !{!"branch_weights", i32 457, i32 99}
 ; Should be 457 total left (original total count 556, minus 99 from specialized
-; value 1, which is removed from VP array. Also, we only end up with 5 total
-; values, since the default max number of promotions is 5 and therefore
-; the rest of the values are ignored when extracting the VP metadata.
-; MEMOP_OPT: [[NEWVP]] = !{!"VP", i32 1, i64 457, i64 2, i64 88, i64 3, i64 77, i64 9, i64 72, i64 4, i64 66}
+; value 0, which is removed from VP array. This should preserve all unpromoted values.
+; MEMOP_OPT: [[NEWVP]] = !{!"VP", i32 1, i64 457, i64 2, i64 88, i64 3, i64 77, i64 9, i64 72, i64 4, i64 66, i64 5, i64 55, i64 6, i64 44, i64 7, i64 33, i64 8, i64 22}
 
 !llvm.module.flags = !{!0}
 
 !0 = !{i32 1, !"ProfileSummary", !1}
 !1 = !{!2, !3, !4, !5, !6, !7, !8, !9}
 !2 = !{!"ProfileFormat", !"InstrProf"}
 !3 = !{!"TotalCount", i64 579}
 !4 = !{!"MaxCount", i64 556}

llvm/test/Transforms/PGOProfile/memop_size_opt_skip_ranges_promote_three.ll

+; RUN: opt < %s -pgo-memop-opt -pgo-memop-count-threshold=100 -pgo-memop-percent-threshold=10 -S | FileCheck %s
+; RUN: opt < %s -passes=pgo-memop-opt -pgo-memop-count-threshold=100 -pgo-memop-percent-threshold=10 -S | FileCheck %s
+
+define void @foo(i8* %dst, i8* %src, i8* %dst2, i8* %src2, i64 %n) !prof !27 {
+entry:
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src, i64 %n, i1 false), !prof !28
+  ret void
+}
+
+; CHECK:  switch i64 %n, label %[[DEFAULT_LABEL:.*]] [
+; CHECK:    i64 0, label %[[CASE_0_LABEL:.*]]
+; CHECK:    i64 1, label %[[CASE_1_LABEL:.*]]
+; CHECK:    i64 2, label %[[CASE_2_LABEL:.*]]
+; CHECK:  ], !prof [[SWITCH_BW:![0-9]+]]
+; CHECK: [[CASE_0_LABEL]]:
+; CHECK:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src, i64 0, i1 false)
+; CHECK:   br label %[[MERGE_LABEL:.*]]
+; CHECK: [[CASE_1_LABEL]]:
+; CHECK:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src, i64 1, i1 false)
+; CHECK:   br label %[[MERGE_LABEL:.*]]
+; CHECK: [[CASE_2_LABEL]]:
+; CHECK:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src, i64 2, i1 false)
+; CHECK:   br label %[[MERGE_LABEL:.*]]
+; CHECK: [[DEFAULT_LABEL]]:
+; CHECK:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src, i64 %n, i1 false), !prof [[NEWVP:![0-9]+]]
+; CHECK:   br label %[[MERGE_LABEL]]
+; CHECK: [[MERGE_LABEL]]:
+; CHECK:   ret void
+
+; It should skip range values 9, 17, 33, 65, 129 and promote (up to) three values, 0,
+; 1, 2 (not 3), and preserve all unpromoted values in the new VP metadata.
+; CHECK: [[SWITCH_BW]] = !{!"branch_weights", i32 524, i32 101, i32 101, i32 101}
+; CHECK: [[NEWVP]] = !{!"VP", i32 1, i64 524, i64 9, i64 104, i64 17, i64 103, i64 33, i64 103, i64 65, i64 102, i64 129, i64 102, i64 3, i64 101}
+
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i1)
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 1, !"ProfileSummary", !1}
+!1 = !{!2, !3, !4, !5, !6, !7, !8, !9}
+!2 = !{!"ProfileFormat", !"InstrProf"}
+!3 = !{!"TotalCount", i64 579}
+!4 = !{!"MaxCount", i64 556}
+!5 = !{!"MaxInternalCount", i64 20}
+!6 = !{!"MaxFunctionCount", i64 556}
+!7 = !{!"NumCounts", i64 6}
+!8 = !{!"NumFunctions", i64 3}
+!9 = !{!"DetailedSummary", !10}
+!10 = !{!11, !12, !13, !14, !15, !16, !16, !17, !17, !18, !19, !20, !21, !22, !23, !24, !25, !26}
+!11 = !{i32 10000, i64 556, i32 1}
+!12 = !{i32 100000, i64 556, i32 1}
+!13 = !{i32 200000, i64 556, i32 1}
+!14 = !{i32 300000, i64 556, i32 1}
+!15 = !{i32 400000, i64 556, i32 1}
+!16 = !{i32 500000, i64 556, i32 1}
+!17 = !{i32 600000, i64 556, i32 1}
+!18 = !{i32 700000, i64 556, i32 1}
+!19 = !{i32 800000, i64 556, i32 1}
+!20 = !{i32 900000, i64 556, i32 1}
+!21 = !{i32 950000, i64 556, i32 1}
+!22 = !{i32 990000, i64 20, i32 2}
+!23 = !{i32 999000, i64 1, i32 5}
+!24 = !{i32 999900, i64 1, i32 5}
+!25 = !{i32 999990, i64 1, i32 5}
+!26 = !{i32 999999, i64 1, i32 5}
+!27 = !{!"function_entry_count", i64 827}
+!28 = !{!"VP", i32 1, i64 827, i64 9, i64 104, i64 17, i64 103, i64 33, i64 103, i64 65, i64 102, i64 129, i64 102, i64 0, i64 101, i64 1, i64 101, i64 2, i64 101, i64 3, i64 101}