[llvm] Avoid BlockFrequency overflow problems (PR #66280)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-analysis
            
<details>
<summary>Changes</summary>
Multiplying raw block frequency with an integer carries a high risk of overflow.

- Introduce a new `BlockFrequency::mul` function returning a `bool` indicating overflow.
- Mark function with `__attribute__((warn_unused_result))` to avoid users accidentally ignoring the indicator.
- Fix two instances where overflow were leading to wrong results for me.
--
Full diff: https://github.com/llvm/llvm-project/pull/66280.diff

5 Files Affected:

- (modified) llvm/include/llvm/Support/BlockFrequency.h (+8) 
- (modified) llvm/include/llvm/Support/Compiler.h (+8) 
- (modified) llvm/lib/Analysis/InlineCost.cpp (+6-5) 
- (modified) llvm/lib/CodeGen/CodeGenPrepare.cpp (+5-5) 
- (modified) llvm/lib/Support/BlockFrequency.cpp (+8) 


<pre>
diff --git a/llvm/include/llvm/Support/BlockFrequency.h b/llvm/include/llvm/Support/BlockFrequency.h
index 6c624d7dad7d801..1711fb592485b4c 100644
--- a/llvm/include/llvm/Support/BlockFrequency.h
+++ b/llvm/include/llvm/Support/BlockFrequency.h
@@ -16,6 +16,8 @@
 #include <cassert>
 #include <cstdint>
 
+#include "llvm/Support/Compiler.h"
+
 namespace llvm {
 
 class BranchProbability;
@@ -76,6 +78,12 @@ class BlockFrequency {
     return NewFreq;
   }
 
+  /// Multiplies frequency with `Factor` and stores the result into `Result`.
+  /// Returns `true` if an overflow occured. Overflows are common and should be
+  /// checked by all callers.
+  bool mul(uint64_t Factor,
+           BlockFrequency *Result) const LLVM_WARN_UNUSED_RESULT;
+
   /// Shift block frequency to the right by count digits saturating to 1.
   BlockFrequency &operator>>=(const unsigned count) {
     // Frequency can never be 0 by design.
diff --git a/llvm/include/llvm/Support/Compiler.h b/llvm/include/llvm/Support/Compiler.h
index 12afe90f8facd47..9527e377317ac33 100644
--- a/llvm/include/llvm/Support/Compiler.h
+++ b/llvm/include/llvm/Support/Compiler.h
@@ -269,6 +269,14 @@
 #define LLVM_ATTRIBUTE_RETURNS_NOALIAS
 #endif
 
+/// Mark a function whose return value should not be ignored. Doing so without
+/// a `[[maybe_unused]]` produces a warning if supported by the compiler.
+#if __has_attribute(warn_unused_result)
+#define LLVM_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
+#else
+#define LLVM_WARN_UNUSED_RESULT
+#endif
+
 /// LLVM_FALLTHROUGH - Mark fallthrough cases in switch statements.
 #if defined(__cplusplus) && __cplusplus > 201402L && LLVM_HAS_CPP_ATTRIBUTE(fallthrough)
 #define LLVM_FALLTHROUGH [[fallthrough]]
diff --git a/llvm/lib/Analysis/InlineCost.cpp b/llvm/lib/Analysis/InlineCost.cpp
index a9de1dde7c7f717..d921047d6466f52 100644
--- a/llvm/lib/Analysis/InlineCost.cpp
+++ b/llvm/lib/Analysis/InlineCost.cpp
@@ -118,7 +118,7 @@ static cl::opt<int> ColdCallSiteRelFreq(
              "entry frequency, for a callsite to be cold in the absence of "
              "profile information."));
 
-static cl::opt<int> HotCallSiteRelFreq(
+static cl::opt<uint64_t> HotCallSiteRelFreq(
     "hot-callsite-rel-freq", cl::Hidden, cl::init(60),
     cl::desc("Minimum block frequency, expressed as a multiple of caller&#x27;s "
              "entry frequency, for a callsite to be hot in the absence of "
@@ -1820,10 +1820,11 @@ InlineCostCallAnalyzer::getHotCallSiteThreshold(CallBase &Call,
   // potentially cache the computation of scaled entry frequency, but the added
   // complexity is not worth it unless this scaling shows up high in the
   // profiles.
-  auto CallSiteBB = Call.getParent();
-  auto CallSiteFreq = CallerBFI->getBlockFreq(CallSiteBB).getFrequency();
-  auto CallerEntryFreq = CallerBFI->getEntryFreq();
-  if (CallSiteFreq >= CallerEntryFreq * HotCallSiteRelFreq)
+  const BasicBlock *CallSiteBB = Call.getParent();
+  BlockFrequency CallSiteFreq = CallerBFI->getBlockFreq(CallSiteBB);
+  BlockFrequency CallerEntryFreq = CallerBFI->getEntryFreq();
+  BlockFrequency Limit;
+  if (!CallerEntryFreq.mul(HotCallSiteRelFreq, &Limit) && CallSiteFreq >= Limit)
     return Params.LocallyHotCallSiteThreshold;
 
   // Otherwise treat it normally.
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index f07fc4fc52bffba..e24361c1f93970d 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -198,7 +198,7 @@ static cl::opt<bool> BBSectionsGuidedSectionPrefix(
              "impacted, i.e., their prefixes will be decided by FDO/sampleFDO "
              "profiles."));
 
-static cl::opt<unsigned> FreqRatioToSkipMerge(
+static cl::opt<uint64_t> FreqRatioToSkipMerge(
     "cgp-freq-ratio-to-skip-merge", cl::Hidden, cl::init(2),
     cl::desc("Skip merging empty blocks if (frequency of empty block) / "
              "(frequency of destination block) is greater than this ratio"));
@@ -978,16 +978,16 @@ bool CodeGenPrepare::isMergingEmptyBlockProfitable(BasicBlock *BB,
   if (SameIncomingValueBBs.count(Pred))
     return true;
 
-  BlockFrequency PredFreq = BFI->getBlockFreq(Pred);
-  BlockFrequency BBFreq = BFI->getBlockFreq(BB);
+  BlockFrequency PredFreq = BFI->getBlockFreq(Pred).getFrequency();
+  BlockFrequency BBFreq = BFI->getBlockFreq(BB).getFrequency();
 
   for (auto *SameValueBB : SameIncomingValueBBs)
     if (SameValueBB->getUniquePredecessor() == Pred &&
         DestBB == findDestBlockOfMergeableEmptyBlock(SameValueBB))
       BBFreq += BFI->getBlockFreq(SameValueBB);
 
-  return PredFreq.getFrequency() <=
-         BBFreq.getFrequency() * FreqRatioToSkipMerge;
+  BlockFrequency Limit;
+  return !BBFreq.mul(FreqRatioToSkipMerge, &Limit) && PredFreq <= Limit;
 }
 
 /// Return true if we can merge BB into DestBB if there is a single
diff --git a/llvm/lib/Support/BlockFrequency.cpp b/llvm/lib/Support/BlockFrequency.cpp
index a4a1e477d9403f7..08fe3ef6061ecae 100644
--- a/llvm/lib/Support/BlockFrequency.cpp
+++ b/llvm/lib/Support/BlockFrequency.cpp
@@ -12,6 +12,7 @@
 
 #include "llvm/Support/BlockFrequency.h"
 #include "llvm/Support/BranchProbability.h"
+#include "llvm/Support/MathExtras.h"
 
 using namespace llvm;
 
@@ -36,3 +37,10 @@ BlockFrequency BlockFrequency::operator/(BranchProbability Prob) const {
   Freq /= Prob;
   return Freq;
 }
+
+bool BlockFrequency::mul(uint64_t Factor, BlockFrequency *Result) const {
+  bool Overflow;
+  uint64_t ResultFrequency = SaturatingMultiply(Frequency, Factor, &Overflow);
+  *Result = BlockFrequency(ResultFrequency);
+  return Overflow;
+}
</pre>
</details>


https://github.com/llvm/llvm-project/pull/66280