[llvm] Avoid BlockFrequency overflow problems (PR #66280)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 13 12:50:30 PDT 2023
llvmbot wrote:
<!--IGNORE-->
>
><!--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'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>
Error: Command failed due to missing milestone.
https://github.com/llvm/llvm-project/pull/66280
More information about the llvm-commits
mailing list