Skip to content

Conversation

@artagnon
Copy link
Contributor

@artagnon artagnon commented Mar 4, 2025

getLoopEstimatedTripCount returns std::nullopt when the trip count would overflow the return type, but since it is an estimate anyway, we might as well saturate at UINT_MAX, improving results.

getLoopEstimatedTripCount returns std::nullopt when the trip count would
overflow the return type, but since it is an estimate anyway, we might
as well saturate at UINT_MAX, improving results.
@llvmbot
Copy link
Member

llvmbot commented Mar 4, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Ramkumar Ramachandra (artagnon)

Changes

getLoopEstimatedTripCount returns std::nullopt when the trip count would overflow the return type, but since it is an estimate anyway, we might as well saturate at UINT_MAX, improving results.


Full diff: https://github.com/llvm/llvm-project/pull/129683.diff

3 Files Affected:

  • (modified) llvm/include/llvm/Transforms/Utils/LoopUtils.h (+2-3)
  • (modified) llvm/lib/Transforms/Utils/LoopUtils.cpp (+2-2)
  • (modified) llvm/test/Transforms/LoopVectorize/AArch64/low_trip_memcheck_cost.ll (+2-2)
diff --git a/llvm/include/llvm/Transforms/Utils/LoopUtils.h b/llvm/include/llvm/Transforms/Utils/LoopUtils.h
index f36400cb5613a..8f4c0c88336ac 100644
--- a/llvm/include/llvm/Transforms/Utils/LoopUtils.h
+++ b/llvm/include/llvm/Transforms/Utils/LoopUtils.h
@@ -318,9 +318,8 @@ void addStringMetadataToLoop(Loop *TheLoop, const char *MDString,
 /// Returns a loop's estimated trip count based on branch weight metadata.
 /// In addition if \p EstimatedLoopInvocationWeight is not null it is
 /// initialized with weight of loop's latch leading to the exit.
-/// Returns a valid positive trip count, or std::nullopt when a meaningful
-/// estimate cannot be made (including when the trip count wouldn't fit in the
-/// result type).
+/// Returns a valid positive trip count, saturated at UINT_MAX, or std::nullopt
+/// when a meaningful estimate cannot be made.
 std::optional<unsigned>
 getLoopEstimatedTripCount(Loop *L,
                           unsigned *EstimatedLoopInvocationWeight = nullptr);
diff --git a/llvm/lib/Transforms/Utils/LoopUtils.cpp b/llvm/lib/Transforms/Utils/LoopUtils.cpp
index ec1692a484ce0..42c70d2c163b5 100644
--- a/llvm/lib/Transforms/Utils/LoopUtils.cpp
+++ b/llvm/lib/Transforms/Utils/LoopUtils.cpp
@@ -843,9 +843,9 @@ static std::optional<unsigned> getEstimatedTripCount(BranchInst *ExitingBranch,
   // edge exiting the loop, rounded to nearest.
   uint64_t ExitCount = llvm::divideNearest(LoopWeight, ExitWeight);
 
-  // When ExitCount + 1 would wrap in unsigned, return std::nullopt instead.
+  // When ExitCount + 1 would wrap in unsigned, saturate at UINT_MAX.
   if (ExitCount >= std::numeric_limits<unsigned>::max())
-    return std::nullopt;
+    return std::numeric_limits<unsigned>::max();
 
   // Estimated trip count is one plus estimated exit count.
   return ExitCount + 1;
diff --git a/llvm/test/Transforms/LoopVectorize/AArch64/low_trip_memcheck_cost.ll b/llvm/test/Transforms/LoopVectorize/AArch64/low_trip_memcheck_cost.ll
index c685655f67a6b..611b980999bfe 100644
--- a/llvm/test/Transforms/LoopVectorize/AArch64/low_trip_memcheck_cost.ll
+++ b/llvm/test/Transforms/LoopVectorize/AArch64/low_trip_memcheck_cost.ll
@@ -180,8 +180,8 @@ outer.exit:
 define void @outer_pgo_minus1(ptr nocapture noundef %a, ptr nocapture noundef readonly %b, i64 noundef %m, i64 noundef %n) {
 ; CHECK-LABEL: LV: Checking a loop in 'outer_pgo_minus1'
 ; CHECK:      Calculating cost of runtime checks:
-; CHECK:      We expect runtime memory checks to be hoisted out of the outer loop. Cost reduced from 6 to 3
-; CHECK:      Total cost of runtime checks: 3
+; CHECK:      We expect runtime memory checks to be hoisted out of the outer loop. Cost reduced from 6 to 1
+; CHECK:      Total cost of runtime checks: 1
 ; CHECK-NEXT: LV: Minimum required TC for runtime checks to be profitable:16
 entry:
   br label %outer.loop

Copy link
Contributor

@david-arm david-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Seems sensible to me.

@artagnon artagnon merged commit 03da079 into llvm:main Mar 5, 2025
13 checks passed
@artagnon artagnon deleted the looputils-tc-sat branch March 5, 2025 18:19
Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up.

Should the title say Saturate at UINT_MAX?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants