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

[LV] Add option to still enable the legacy cost model. #99536

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

fhahn
Copy link
Contributor

@fhahn fhahn commented Jul 18, 2024

This patch adds a new temporary option to still use the legacy cost model after #92555. It defaults to false and the only intended use is to adjust the default to true in the soon-to-be-cut release branch.

This patch adds a new temporary option to still use the legacy cost
model after llvm#92555.
It defaults to false and the only intended use is to adjust the
default to true in the soon-to-be-cut release branch.
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 18, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

This patch adds a new temporary option to still use the legacy cost model after #92555. It defaults to false and the only intended use is to adjust the default to true in the soon-to-be-cut release branch.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+20-10)
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 748db418fee8c..7602c1021663b 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -205,6 +205,11 @@ static cl::opt<unsigned> VectorizeMemoryCheckThreshold(
     "vectorize-memory-check-threshold", cl::init(128), cl::Hidden,
     cl::desc("The maximum allowed number of runtime memory checks"));
 
+static cl::opt<bool> UseLegacyCostModel(
+    "vectorize-use-legacy-cost-model", cl::init(false), cl::Hidden,
+    cl::desc("Use the legacy cost model instead of the VPlan-based cost model. "
+             "This option will be removed in the future."));
+
 // Option prefer-predicate-over-epilogue indicates that an epilogue is undesired,
 // that predication is preferred, and this lists all options. I.e., the
 // vectorizer will try to fold the tail-loop (epilogue) into the vector body
@@ -10318,8 +10323,9 @@ bool LoopVectorizePass::processLoop(Loop *L) {
       InnerLoopUnroller Unroller(L, PSE, LI, DT, TLI, TTI, AC, ORE, IC, &LVL,
                                  &CM, BFI, PSI, Checks);
 
-      VPlan &BestPlan = LVP.getBestPlan();
-      assert(BestPlan.hasScalarVFOnly() &&
+      VPlan &BestPlan =
+          UseLegacyCostModel ? LVP.getBestPlanFor(VF.Width) : LVP.getBestPlan();
+      assert((UseLegacyCostModel || BestPlan.hasScalarVFOnly()) &&
              "VPlan cost model and legacy cost model disagreed");
       LVP.executePlan(VF.Width, IC, BestPlan, Unroller, DT, false);
 
@@ -10436,14 +10442,18 @@ bool LoopVectorizePass::processLoop(Loop *L) {
         if (!MainILV.areSafetyChecksAdded())
           DisableRuntimeUnroll = true;
       } else {
-        VPlan &BestPlan = LVP.getBestPlan();
-        assert(size(BestPlan.vectorFactors()) == 1 &&
-               "Plan should have a single VF");
-        ElementCount Width = *BestPlan.vectorFactors().begin();
-        LLVM_DEBUG(dbgs() << "VF picked by VPlan cost model: " << Width
-                          << "\n");
-        assert(VF.Width == Width &&
-               "VPlan cost model and legacy cost model disagreed");
+        ElementCount Width = VF.Width;
+        VPlan &BestPlan =
+            UseLegacyCostModel ? LVP.getBestPlanFor(Width) : LVP.getBestPlan();
+        if (!UseLegacyCostModel) {
+          assert(size(BestPlan.vectorFactors()) == 1 &&
+                 "Plan should have a single VF");
+          Width = *BestPlan.vectorFactors().begin();
+          LLVM_DEBUG(dbgs()
+                     << "VF picked by VPlan cost model: " << Width << "\n");
+          assert(VF.Width == Width &&
+                 "VPlan cost model and legacy cost model disagreed");
+        }
         InnerLoopVectorizer LB(L, PSE, LI, DT, TLI, TTI, AC, ORE, Width,
                                VF.MinProfitableTripCount, IC, &LVL, &CM, BFI,
                                PSI, Checks);

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.

Looks reasonable.

@fhahn fhahn merged commit 9ba5244 into llvm:main Jul 19, 2024
8 of 10 checks passed
@fhahn fhahn deleted the lv-legacy-cost-model-option branch July 19, 2024 17:48
fhahn added a commit to fhahn/llvm-project that referenced this pull request Jul 23, 2024
As discussed in  llvm#92555 flip
the default for the option added in
llvm#99536 to true.

This restores the original behavior for the release branch to give the
VPlan-based cost model more time to mature on main.
tru pushed a commit that referenced this pull request Jul 23, 2024
As discussed in  #92555 flip
the default for the option added in
#99536 to true.

This restores the original behavior for the release branch to give the
VPlan-based cost model more time to mature on main.
sgundapa pushed a commit to sgundapa/upstream_effort that referenced this pull request Jul 23, 2024
This patch adds a new temporary option to still use the legacy cost
model after llvm#92555. It defaults
to false and the only intended use is to adjust the default to true in
the soon-to-be-cut release branch.

PR: llvm#99536
fhahn added a commit that referenced this pull request Jul 24, 2024
This reverts commit 9ba5244.

Remove the recently added temporary option vectorize-use-legacy-cost-model
as discussed on the PR adding it, now that we branched for 19.x.
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
This patch adds a new temporary option to still use the legacy cost
model after #92555. It defaults
to false and the only intended use is to adjust the default to true in
the soon-to-be-cut release branch.

PR: #99536

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251192
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
This reverts commit 9ba5244.

Remove the recently added temporary option vectorize-use-legacy-cost-model
as discussed on the PR adding it, now that we branched for 19.x.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250748
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