[LV] Prevent query the computeCost() when VF=1 in emitInvalidCostRemarks(). #117288

ElvisWang123 · 2024-11-22T05:07:46Z

We should only query the computeCost() when the VF is vector.

llvmbot · 2024-11-22T05:08:19Z

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Elvis Wang (ElvisWang123)

Changes

We should only query the computeCost() when the VF is vector.

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

1 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+3)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index d13770a35c108f..5a61a6f83e3764 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -4373,6 +4373,9 @@ void LoopVectorizationPlanner::emitInvalidCostRemarks(
   SmallVector<RecipeVFPair> InvalidCosts;
   for (const auto &Plan : VPlans) {
     for (ElementCount VF : Plan->vectorFactors()) {
+      if (VF.isScalar())
+        continue;
+
       VPCostContext CostCtx(CM.TTI, *CM.TLI, Legal->getWidestInductionType(),
                             CM);
       precomputeCosts(*Plan, VF, CostCtx);

llvmbot · 2024-11-22T05:08:20Z

@llvm/pr-subscribers-vectorizers

Author: Elvis Wang (ElvisWang123)

Changes

We should only query the computeCost() when the VF is vector.

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

1 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+3)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index d13770a35c108f..5a61a6f83e3764 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -4373,6 +4373,9 @@ void LoopVectorizationPlanner::emitInvalidCostRemarks(
   SmallVector<RecipeVFPair> InvalidCosts;
   for (const auto &Plan : VPlans) {
     for (ElementCount VF : Plan->vectorFactors()) {
+      if (VF.isScalar())
+        continue;
+
       VPCostContext CostCtx(CM.TTI, *CM.TLI, Legal->getWidestInductionType(),
                             CM);
       precomputeCosts(*Plan, VF, CostCtx);

ElvisWang123 · 2024-12-04T06:10:28Z

Soft ping!

david-arm

LGTM! Seems sensible.

fhahn · 2024-12-12T23:15:44Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

@@ -4372,6 +4372,9 @@ void LoopVectorizationPlanner::emitInvalidCostRemarks(
  SmallVector<RecipeVFPair> InvalidCosts;
  for (const auto &Plan : VPlans) {
    for (ElementCount VF : Plan->vectorFactors()) {
+      if (VF.isScalar())


Could you add a comment here why we skip scalar VFs? Presumably because we don’t expect them to have invalid costs?

Updated comments.

Currently, we cannot query the vplan-based cost model with scalar VF because we use lots of cast<VectorType>(toVectorTy(SrcTy, VF)) in computeCost().

ElvisWang123 · 2024-12-18T07:35:35Z

Found a test case extracted from TSVC s311.
https://godbolt.org/z/acj6M6nfE

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

david-arm · 2024-12-20T09:57:46Z

llvm/test/Transforms/LoopVectorize/RISCV/remark-reductions.ll

@@ -0,0 +1,65 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt < %s -mtriple=riscv64 -mattr=+v -p loop-vectorize -pass-remarks-analysis=loop-vectorize -S | FileCheck %s


This doesn't feel quite right, since you're passing -pass-remarks-analysis=loop-vectorize but not testing the output. Given this test is about remarks, I think we should check the output and also ensure we don't see the VF=1 case.

Passing -pass-remarks-analysis=loop-vectorize just for triggering emitInvalidCostRemarks() and make sure the compiler don't crash. I don't have any ideas about testing the remark output.

Something I can imagine is that using --debug-only=loop-vecotorize to print the cost. But it always using the legacy cost model to calculate/print the scalar cost.

Do you have better suggestion to test this case?

You could also check the remark output, assuming there is any? Or of not, check that there is no remark output

Update test, thanks!

fhahn · 2025-02-04T08:42:13Z

llvm/test/Transforms/LoopVectorize/RISCV/remark-reductions.ll

@@ -0,0 +1,65 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt < %s -mtriple=riscv64 -mattr=+v -p loop-vectorize -pass-remarks-analysis=loop-vectorize -S | FileCheck %s


You could also check the remark output, assuming there is any? Or of not, check that there is no remark output

fhahn · 2025-02-04T08:42:44Z

llvm/test/Transforms/LoopVectorize/RISCV/remark-reductions.ll

+loop:
+  %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop ]
+  %red = phi float [ 0.000000e+00, %entry ], [ %red.next, %loop ]
+  %red.next = fadd float 0.000000e+00, %red


this is a no-op reduction, can this also reproduce with a 'real' reductions?

Yes, update test case to real reduction.
https://godbolt.org/z/dj3hrK8fT

…rks().

Becaue the original test case will not vectorized anymore. Update the test case the will hit the bug.

fhahn

LGTM, thanks

…rks(). (llvm#117288) We should only query the computeCost() when the VF is vector.

ElvisWang123 requested review from fhahn, arcbbb and topperc November 22, 2024 05:07

llvmbot added vectorizers llvm:transforms labels Nov 22, 2024

llvmbot added the llvm:analysis label Nov 22, 2024

ElvisWang123 force-pushed the fix-missing-VF-check-in-emitRemark-LV branch from 7619544 to 6f1c1c7 Compare November 22, 2024 14:13

ElvisWang123 force-pushed the fix-missing-VF-check-in-emitRemark-LV branch from 6f1c1c7 to 2e9579d Compare December 4, 2024 00:09

david-arm approved these changes Dec 12, 2024

View reviewed changes

fhahn reviewed Dec 12, 2024

View reviewed changes

ElvisWang123 force-pushed the fix-missing-VF-check-in-emitRemark-LV branch from f30cdb0 to 9c8caeb Compare December 18, 2024 07:33

david-arm reviewed Dec 20, 2024

View reviewed changes

ElvisWang123 force-pushed the fix-missing-VF-check-in-emitRemark-LV branch from 6385ea0 to bb4f8f5 Compare February 4, 2025 07:41

fhahn reviewed Feb 4, 2025

View reviewed changes

ElvisWang123 added 5 commits February 4, 2025 18:51

[LV] Prevent query the computeCost() when VF=1 in emitInvalidCostRema…

6b819b9

…rks().

Add comments to explain why we skip scalar cost.

d197787

Add test case.

37e5559

!fixup typo and update test case.

86ed6b4

Becaue the original test case will not vectorized anymore. Update the test case the will hit the bug.

!Fixup, using real reduction and checks remark output.

5ad2ad5

ElvisWang123 force-pushed the fix-missing-VF-check-in-emitRemark-LV branch from bb4f8f5 to 5ad2ad5 Compare February 5, 2025 02:54

fhahn approved these changes Feb 8, 2025

View reviewed changes

Merge branch 'main' into fix-missing-VF-check-in-emitRemark-LV

b6e603c

ElvisWang123 merged commit 2e3729b into llvm:main Feb 10, 2025
8 checks passed

ElvisWang123 deleted the fix-missing-VF-check-in-emitRemark-LV branch February 10, 2025 00:40

Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025

[LV] Prevent query the computeCost() when VF=1 in emitInvalidCostRema…

4e7205c

…rks(). (llvm#117288) We should only query the computeCost() when the VF is vector.

		@@ -0,0 +1,65 @@
		; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
		; RUN: opt < %s -mtriple=riscv64 -mattr=+v -p loop-vectorize -pass-remarks-analysis=loop-vectorize -S \| FileCheck %s

[LV] Prevent query the computeCost() when VF=1 in emitInvalidCostRemarks(). #117288

[LV] Prevent query the computeCost() when VF=1 in emitInvalidCostRemarks(). #117288

Uh oh!

Conversation

ElvisWang123 commented Nov 22, 2024

Uh oh!

llvmbot commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 22, 2024

Uh oh!

ElvisWang123 commented Dec 4, 2024

Uh oh!

david-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElvisWang123 commented Dec 18, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Nov 22, 2024 •

edited

Loading