Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make 'UnrollMaxUpperBound' to be overridable by target. #76029

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

boxu-zhang
Copy link
Contributor

The UnrollMaxUpperBound should be target dependent, since different chips provide different register set which brings different ability of storing more temporary values of a program. So I add a MaxUpperBound value in UnrollingPreference which can be override by targets. All uses of UnrollMaxUpperBound are replaced with UP.MaxUpperBound.

The default value is still 8 and the command line argument '--unroll-max-upperbound' takes final effect if provided.

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 20, 2023

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: boxu.zhang (boxu-zhang)

Changes

The UnrollMaxUpperBound should be target dependent, since different chips provide different register set which brings different ability of storing more temporary values of a program. So I add a MaxUpperBound value in UnrollingPreference which can be override by targets. All uses of UnrollMaxUpperBound are replaced with UP.MaxUpperBound.

The default value is still 8 and the command line argument '--unroll-max-upperbound' takes final effect if provided.


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

2 Files Affected:

  • (modified) llvm/include/llvm/Analysis/TargetTransformInfo.h (+7)
  • (modified) llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp (+6-3)
diff --git a/llvm/include/llvm/Analysis/TargetTransformInfo.h b/llvm/include/llvm/Analysis/TargetTransformInfo.h
index f5114fa40c70ad..3d7191b902b0ea 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfo.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfo.h
@@ -560,6 +560,13 @@ class TargetTransformInfo {
     // (set to UINT_MAX to disable). This does not apply in cases where the
     // loop is being fully unrolled.
     unsigned MaxCount;
+    /// Set the maximum upper bound of trip count. Considering a loop with 'pragma 
+    /// unroll' specified but without 'full' or 'n' field, it depends on the 
+    /// UnrollMaxUpperBound option if the loop trip is unknown. However, the 
+    /// default value is 8 for this option which might not suitable for certain
+    /// target. Allowing the MaxUpperBound to be overrided by a target gives more
+    /// flexiblity on such cases. By default, MaxUpperBound uses UnrollMaxUpperBound
+    unsigned MaxUpperBound;
     /// Set the maximum unrolling factor for full unrolling. Like MaxCount, but
     /// applies even if full unrolling is selected. This allows a target to fall
     /// back to Partial unrolling if full unrolling is above FullUnrollMaxCount.
diff --git a/llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp b/llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
index f14541a1a037e6..ddc532ea24e275 100644
--- a/llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
@@ -200,6 +200,7 @@ TargetTransformInfo::UnrollingPreferences llvm::gatherUnrollingPreferences(
   UP.Count = 0;
   UP.DefaultUnrollRuntimeCount = 8;
   UP.MaxCount = std::numeric_limits<unsigned>::max();
+  UP.MaxUpperBound = UnrollMaxUpperBound; 
   UP.FullUnrollMaxCount = std::numeric_limits<unsigned>::max();
   UP.BEInsns = 2;
   UP.Partial = false;
@@ -237,6 +238,8 @@ TargetTransformInfo::UnrollingPreferences llvm::gatherUnrollingPreferences(
     UP.MaxPercentThresholdBoost = UnrollMaxPercentThresholdBoost;
   if (UnrollMaxCount.getNumOccurrences() > 0)
     UP.MaxCount = UnrollMaxCount;
+  if (UnrollMaxUpperBound.getNumOccurrences() > 0)
+    UP.MaxUpperBound = UnrollMaxUpperBound;
   if (UnrollFullMaxCount.getNumOccurrences() > 0)
     UP.FullUnrollMaxCount = UnrollFullMaxCount;
   if (UnrollAllowPartial.getNumOccurrences() > 0)
@@ -777,7 +780,7 @@ shouldPragmaUnroll(Loop *L, const PragmaInfo &PInfo,
     return TripCount;
 
   if (PInfo.PragmaEnableUnroll && !TripCount && MaxTripCount &&
-      MaxTripCount <= UnrollMaxUpperBound)
+      MaxTripCount <= UP.MaxUpperBound)
     return MaxTripCount;
 
   // if didn't return until here, should continue to other priorties
@@ -952,7 +955,7 @@ bool llvm::computeUnrollCount(
   // cost of exact full unrolling.  As such, if we have an exact count and
   // found it unprofitable, we'll never chose to bounded unroll.
   if (!TripCount && MaxTripCount && (UP.UpperBound || MaxOrZero) &&
-      MaxTripCount <= UnrollMaxUpperBound) {
+      MaxTripCount <= UP.MaxUpperBound) {
     UP.Count = MaxTripCount;
     if (auto UnrollFactor = shouldFullUnroll(L, TTI, DT, SE, EphValues,
                                              MaxTripCount, UCE, UP)) {
@@ -1026,7 +1029,7 @@ bool llvm::computeUnrollCount(
   }
 
   // Don't unroll a small upper bound loop unless user or TTI asked to do so.
-  if (MaxTripCount && !UP.Force && MaxTripCount < UnrollMaxUpperBound) {
+  if (MaxTripCount && !UP.Force && MaxTripCount < UP.MaxUpperBound) {
     UP.Count = 0;
     return false;
   }

Copy link

github-actions bot commented Dec 20, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Making this a TTI hook sounds reasonable to me.

Comment on lines 563 to 568
/// Set the maximum upper bound of trip count. Considering a loop with 'pragma
/// unroll' specified but without 'full' or 'n' field, it depends on the
/// UnrollMaxUpperBound option if the loop trip is unknown. However, the
/// default value is 8 for this option which might not suitable for certain
/// target. Allowing the MaxUpperBound to be overrided by a target gives more
/// flexiblity on such cases. By default, MaxUpperBound uses UnrollMaxUpperBound
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Set the maximum upper bound of trip count. Considering a loop with 'pragma
/// unroll' specified but without 'full' or 'n' field, it depends on the
/// UnrollMaxUpperBound option if the loop trip is unknown. However, the
/// default value is 8 for this option which might not suitable for certain
/// target. Allowing the MaxUpperBound to be overrided by a target gives more
/// flexiblity on such cases. By default, MaxUpperBound uses UnrollMaxUpperBound
/// Set the maximum number of iterations to fully unroll if only an upper bound
/// on the trip count is known.

I don't think the rest of the discussion really belongs in this comment. Note that this option also affects the non-pragma case, so mentioning that is misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments update, can you please review it again and merge if it's OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's OK now. Can you please merge it?

@boxu-zhang boxu-zhang force-pushed the boxu.zhang.llvm branch 2 times, most recently from 8a139dc to fe4fc86 Compare December 20, 2023 09:37
@nikic
Copy link
Contributor

nikic commented Dec 20, 2023

Can you please fix the clang-format failure? It's due to trailing whitespace.

@boxu-zhang
Copy link
Contributor Author

Can you please fix the clang-format failure? It's due to trailing whitespace.

Updated.

…alue is still 8 and the command line argument '--unroll-max-upperbound' takes final effect if provided.
@nikic nikic merged commit d3ef867 into llvm:main Dec 21, 2023
4 checks passed
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.

3 participants