[PATCH] D61540: [PGO] Use sum of count values to fix func entry count and add a check to verify BFI counts

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1564
+  auto SumCount = APFloat::getZero(APFloat::IEEEdouble());
+  auto SumBFCount = APFloat::getZero(APFloat::IEEEdouble());
+  for (auto &BBI : F) {
----------------
assert here that function entry count is already set.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1568
+    uint64_t BFICountValue = 0;
+    if (!Func.getBBInfo(&BBI).CountValid)
+      continue;
----------------
In what situation can this happen?


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1571
+    auto BFICount = NBFI.getBlockProfileCount(&BBI);
+    if (!BFICount)
+      continue;
----------------
how can this be possible?


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1579
+  }
+  if (!SumCount.isZero()) {
+    double Scale = 1.0;
----------------
maybe early return?


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1582
+    if (SumBFCount.compare(APFloat(1.0)) == APFloat::cmpGreaterThan &&
+        SumBFCount.compare(SumCount) != APFloat::cmpEqual)
+      Scale = (SumCount / SumBFCount).convertToDouble();
----------------
negate the logic and early return?


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1583
+        SumBFCount.compare(SumCount) != APFloat::cmpEqual)
+      Scale = (SumCount / SumBFCount).convertToDouble();
+
----------------
assert SumBFCount is not zero


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1585
+
+    if (Scale >= 1.01 || Scale <= 0.99) {
+      uint64_t FuncEntryCount = Func.getBBInfo(&*F.begin()).CountValue;
----------------
early return?


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1588
+      uint64_t NewEntryCount = FuncEntryCount;
+      if (NewEntryCount == 0)
+        NewEntryCount = 1;
----------------
Is this handled by the next statements?


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1602
+  }
+  F.setEntryCount(ProfileCount(0, Function::PCT_Real));
+}
----------------
Is this needed?


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

https://reviews.llvm.org/D61540