[RISCV] Fix crash when unrolling loop containing vector instructions #83384

arcbbb · 2024-02-29T07:10:44Z

When MVT is not a vector type, TCK_CodeSize should return an invalid cost. This patch adds a check in the beginning to make sure all cost kinds return invalid costs consistently.

Before this patch, TCK_CodeSize returns a valid cost on scalar MVT but other cost kinds doesn't.

This fixes the issue #83294 where a loop contains vector instructions and MVT is scalar after type legalization when the vector extension is not enabled,

When MVT is not a vector type, TCK_CodeSize should return an invalid cost. This patch adds a check in the beginning to make sure all cost kinds return invalid costs consistently. Before this patch, TCK_CodeSize returns a valid cost on scalar MVT but other cost kinds doesn't. This fixes the issue llvm#83294 where a loop contains vector instructions and MVT is scalar after type legalization when the vector extension is not enabled,

llvmbot · 2024-02-29T07:11:13Z

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-backend-risc-v

Author: Shih-Po Hung (arcbbb)

Changes

When MVT is not a vector type, TCK_CodeSize should return an invalid cost. This patch adds a check in the beginning to make sure all cost kinds return invalid costs consistently.

Before this patch, TCK_CodeSize returns a valid cost on scalar MVT but other cost kinds doesn't.

This fixes the issue #83294 where a loop contains vector instructions and MVT is scalar after type legalization when the vector extension is not enabled,

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

2 Files Affected:

(modified) llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp (+3)
(added) llvm/test/Analysis/CostModel/RISCV/rvv-invalid-cost.ll (+53)

diff --git a/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp b/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
index f04968d82e86e2..54c7402e4444cd 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
@@ -37,6 +37,9 @@ static cl::opt<unsigned> SLPMaxVF(
 InstructionCost
 RISCVTTIImpl::getRISCVInstructionCost(ArrayRef<unsigned> OpCodes, MVT VT,
                                       TTI::TargetCostKind CostKind) {
+  // Check if the type is valid for all CostKind
+  if (!VT.isVector())
+    return InstructionCost::getInvalid();
   size_t NumInstr = OpCodes.size();
   if (CostKind == TTI::TCK_CodeSize)
     return NumInstr;
diff --git a/llvm/test/Analysis/CostModel/RISCV/rvv-invalid-cost.ll b/llvm/test/Analysis/CostModel/RISCV/rvv-invalid-cost.ll
new file mode 100644
index 00000000000000..8df26d88f52898
--- /dev/null
+++ b/llvm/test/Analysis/CostModel/RISCV/rvv-invalid-cost.ll
@@ -0,0 +1,53 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt < %s -mtriple=riscv64 -mattr=+f,+d --passes=loop-unroll-full -S | FileCheck %s
+
+; Check it doesn' crash when the vector extension is not enabled.
+define void @foo() {
+; CHECK-LABEL: define void @foo(
+; CHECK-SAME: ) #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_BODY:%.*]]
+; CHECK:       for.body:
+; CHECK-NEXT:    [[INDVARS_IV1:%.*]] = phi i64 [ [[INDVARS_IV_NEXT:%.*]], [[FOR_BODY]] ], [ 0, [[ENTRY:%.*]] ]
+; CHECK-NEXT:    [[TMP0:%.*]] = load float, ptr null, align 4
+; CHECK-NEXT:    [[SPLAT_SPLAT_I_I_I:%.*]] = shufflevector <2 x float> zeroinitializer, <2 x float> zeroinitializer, <2 x i32> zeroinitializer
+; CHECK-NEXT:    [[CMP1_I_I_I:%.*]] = fcmp ogt <2 x float> zeroinitializer, zeroinitializer
+; CHECK-NEXT:    [[SPLAT_SPLAT3_I_I_I:%.*]] = shufflevector <2 x i32> zeroinitializer, <2 x i32> zeroinitializer, <2 x i32> zeroinitializer
+; CHECK-NEXT:    [[XOR3_I_I_I_I_I:%.*]] = select <2 x i1> zeroinitializer, <2 x i32> zeroinitializer, <2 x i32> zeroinitializer
+; CHECK-NEXT:    [[TMP1:%.*]] = load float, ptr null, align 4
+; CHECK-NEXT:    [[SPLAT_SPLAT8_I_I_I:%.*]] = shufflevector <2 x float> zeroinitializer, <2 x float> zeroinitializer, <2 x i32> zeroinitializer
+; CHECK-NEXT:    [[SUB_I_I_I:%.*]] = fsub <2 x float> zeroinitializer, zeroinitializer
+; CHECK-NEXT:    [[MUL_I_I_I:%.*]] = shl i64 0, 0
+; CHECK-NEXT:    [[TMP2:%.*]] = load float, ptr null, align 4
+; CHECK-NEXT:    [[SPLAT_SPLAT_I_I_I_I:%.*]] = shufflevector <2 x float> zeroinitializer, <2 x float> zeroinitializer, <2 x i32> zeroinitializer
+; CHECK-NEXT:    [[XOR3_I_I_I_V_I_I:%.*]] = select <2 x i1> zeroinitializer, <2 x float> zeroinitializer, <2 x float> zeroinitializer
+; CHECK-NEXT:    [[INDVARS_IV_NEXT]] = add i64 [[INDVARS_IV1]], 1
+; CHECK-NEXT:    [[EXITCOND:%.*]] = icmp ne i64 [[INDVARS_IV1]], 8
+; CHECK-NEXT:    br i1 [[EXITCOND]], label [[FOR_BODY]], label [[EXIT:%.*]]
+; CHECK:       exit:
+; CHECK-NEXT:    ret void
+;
+entry:
+  br label %for.body
+
+for.body:                                         ; preds = %for.body, %entry
+  %indvars.iv1 = phi i64 [ %indvars.iv.next, %for.body ], [ 0, %entry ]
+  %0 = load float, ptr null, align 4
+  %splat.splat.i.i.i = shufflevector <2 x float> zeroinitializer, <2 x float> zeroinitializer, <2 x i32> zeroinitializer
+  %cmp1.i.i.i = fcmp ogt <2 x float> zeroinitializer, zeroinitializer
+  %splat.splat3.i.i.i = shufflevector <2 x i32> zeroinitializer, <2 x i32> zeroinitializer, <2 x i32> zeroinitializer
+  %xor3.i.i.i.i.i = select <2 x i1> zeroinitializer, <2 x i32> zeroinitializer, <2 x i32> zeroinitializer
+  %1 = load float, ptr null, align 4
+  %splat.splat8.i.i.i = shufflevector <2 x float> zeroinitializer, <2 x float> zeroinitializer, <2 x i32> zeroinitializer
+  %sub.i.i.i = fsub <2 x float> zeroinitializer, zeroinitializer
+  %mul.i.i.i = shl i64 0, 0
+  %2 = load float, ptr null, align 4
+  %splat.splat.i.i.i.i = shufflevector <2 x float> zeroinitializer, <2 x float> zeroinitializer, <2 x i32> zeroinitializer
+  %xor3.i.i.i.v.i.i = select <2 x i1> zeroinitializer, <2 x float> zeroinitializer, <2 x float> zeroinitializer
+  %indvars.iv.next = add i64 %indvars.iv1, 1
+  %exitcond = icmp ne i64 %indvars.iv1, 8
+  br i1 %exitcond, label %for.body, label %exit
+
+exit:                                             ; preds = %for.body
+  ret void
+}

frasercrmck

LGTM, thanks

frasercrmck · 2024-02-29T09:57:36Z

llvm/test/Analysis/CostModel/RISCV/rvv-invalid-cost.ll

@@ -0,0 +1,53 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4


Is rvv-invalid-cost the best name for this test? It might make people think there's an invalid cost when using +v.

or rvv-cost-without-v.ll?

vector-cost-without-v.ll?

Sounds good!

llvm/test/Analysis/CostModel/RISCV/rvv-invalid-cost.ll

frasercrmck · 2024-02-29T09:58:42Z

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp

@@ -37,6 +37,9 @@ static cl::opt<unsigned> SLPMaxVF(
 InstructionCost
 RISCVTTIImpl::getRISCVInstructionCost(ArrayRef<unsigned> OpCodes, MVT VT,


This might be an aside but I find it a little surprising that this function's called getRISCVInstructionCost but it only handles vector instructions. Sounds like it should be getRVVInstructionCost. I might be missing something though.

Good point. Your suggested name appears to be clearer.

lukel97 · 2024-02-29T12:03:12Z

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp

@@ -37,6 +37,9 @@ static cl::opt<unsigned> SLPMaxVF(
 InstructionCost
 RISCVTTIImpl::getRISCVInstructionCost(ArrayRef<unsigned> OpCodes, MVT VT,
                                      TTI::TargetCostKind CostKind) {
+  // Check if the type is valid for all CostKind
+  if (!VT.isVector())
+    return InstructionCost::getInvalid();


Where is this getting called from with a scalar MVT? I would have thought that after scalarization we wouldn't have any vector instructions that would still be calling into this

It is from LoopUnrollPass along the way from TTI.getInstructionCost to TTI.getShuffleCost for

%splat.splat.i.i.i = shufflevector <2 x float> zeroinitializer, <2 x float> zeroinitializer, <2 x i32> zeroinitializer

The data type is <2 x float>, but becomes float after getTypeLegalizationCost(Tp); then enters getRISCVInstructionCost().
I'm just not sure how a vector type can be generated without a vector support.

If it is true, we might consider checking if the type is still vector after legalization at the beginning of TTI functions which accept vector types.

Ah ok, thanks for clarifying. I guess there's nothing stopping a frontend from just emitting vector LLVM IR independently of the autovectorizers.

I'm not strongly opinionated about where we check that the legalized type is vector, I presume both will result in an invalid cost

Yeah, in our case it's because we're compiling OpenCL or SYCL, with vector data types in the source.

…lvm#83384) When MVT is not a vector type, TCK_CodeSize should return an invalid cost. This patch adds a check in the beginning to make sure all cost kinds return invalid costs consistently. Before this patch, TCK_CodeSize returns a valid cost on scalar MVT but other cost kinds doesn't. This fixes the issue llvm#83294 where a loop contains vector instructions and MVT is scalar after type legalization when the vector extension is not enabled, (cherry picked from commit fb67dce)

arcbbb requested review from frasercrmck and topperc February 29, 2024 07:10

llvmbot added backend:RISC-V llvm:analysis labels Feb 29, 2024

arcbbb mentioned this pull request Feb 29, 2024

[LoopUnroll][RISCV] Crash when unrolling loop containing vector instructions #83294

Closed

frasercrmck approved these changes Feb 29, 2024

View reviewed changes

lukel97 reviewed Feb 29, 2024

View reviewed changes

arcbbb added 3 commits February 29, 2024 08:34

Rename test

736edf9

Fix typo

4ec95de

rename the test to vector-cost-without-v.ll

f4e6633

arcbbb merged commit fb67dce into llvm:main Mar 2, 2024

arcbbb deleted the fix-83294 branch March 2, 2024 04:34

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISCV] Fix crash when unrolling loop containing vector instructions #83384

[RISCV] Fix crash when unrolling loop containing vector instructions #83384

arcbbb commented Feb 29, 2024

llvmbot commented Feb 29, 2024 •

edited

Loading

frasercrmck left a comment

frasercrmck Feb 29, 2024

arcbbb Feb 29, 2024

frasercrmck Feb 29, 2024

arcbbb Mar 1, 2024

frasercrmck Feb 29, 2024

arcbbb Feb 29, 2024

lukel97 Feb 29, 2024

arcbbb Feb 29, 2024

lukel97 Feb 29, 2024

frasercrmck Feb 29, 2024

		@@ -0,0 +1,53 @@
		; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4

		@@ -37,6 +37,9 @@ static cl::opt<unsigned> SLPMaxVF(
		InstructionCost
		RISCVTTIImpl::getRISCVInstructionCost(ArrayRef<unsigned> OpCodes, MVT VT,

[RISCV] Fix crash when unrolling loop containing vector instructions #83384

[RISCV] Fix crash when unrolling loop containing vector instructions #83384

Conversation

arcbbb commented Feb 29, 2024

llvmbot commented Feb 29, 2024 • edited Loading

frasercrmck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llvmbot commented Feb 29, 2024 •

edited

Loading