[TTI] getInstructionCost - consistently treat all undef/poison shuffle masks as free

[RKSimon]

#145920 exposed an issue where we were treating undef/poison shuffles as SK_Select kinds

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: Simon Pilgrim (RKSimon)

Changes

#145920 exposed an issue where we were treating undef/poison shuffles as SK_Select kinds

---

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

2 Files Affected:

• (modified) llvm/include/llvm/Analysis/TargetTransformInfoImpl.h (+4)
• (modified) llvm/test/Analysis/CostModel/X86/alternate-shuffle-cost.ll (+19)

diff --git a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
index d513e9472a152..12f87226c5f57 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
@@ -1534,6 +1534,10 @@ class TargetTransformInfoImplCRTPBase : public TargetTransformInfoImplBase {
       ArrayRef<int> Mask = Shuffle->getShuffleMask();
       int NumSubElts, SubIndex;
 
+      // Treat undef/poison mask as free (no matter the length).
+      if (all_of(Mask, [](int M) { return M < 0; }))
+        return TTI::TCC_Free;
+
       // TODO: move more of this inside improveShuffleKindFromMask.
       if (Shuffle->changesLength()) {
         // Treat a 'subvector widening' as a free shuffle.
diff --git a/llvm/test/Analysis/CostModel/X86/alternate-shuffle-cost.ll b/llvm/test/Analysis/CostModel/X86/alternate-shuffle-cost.ll
index dc770f4ff74c8..777fb5b2f57d4 100644
--- a/llvm/test/Analysis/CostModel/X86/alternate-shuffle-cost.ll
+++ b/llvm/test/Analysis/CostModel/X86/alternate-shuffle-cost.ll
@@ -758,3 +758,22 @@ define <32 x i8> @test_v32i8_3(<32 x i8> %a, <32 x i8> %b) {
   %1 = shufflevector <32 x i8> %a, <32 x i8> %b, <32 x i32> <i32 0, i32 1, i32 2, i32 3, i32 36, i32 5, i32 38, i32 7, i32 40, i32 9, i32 42, i32 11, i32 44, i32 13, i32 46, i32 15, i32 48, i32 17, i32 50, i32 19, i32 52, i32 21, i32 54, i32 23, i32 56, i32 25, i32 58, i32 27, i32 60, i32 61, i32 62, i32 63>
   ret <32 x i8> %1
 }
+
+; Treat all undef/poison shuffle masks as free.
+define <2 x i32> @test_v2i32_poison(<2 x i32> %a0, <2 x i32> %a1) {
+; CHECK-LABEL: 'test_v2i32_poison'
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: %s = shufflevector <2 x i32> %a0, <2 x i32> %a1, <2 x i32> poison
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: ret <2 x i32> %s
+;
+  %s = shufflevector <2 x i32> %a0, <2 x i32> %a1, <2 x i32> poison
+  ret <2 x i32> %s
+}
+
+define <4 x float> @test_v4f32_v2f32_poison(<2 x float> %a0, <2 x float> %a1) {
+; CHECK-LABEL: 'test_v4f32_v2f32_poison'
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: %s = shufflevector <2 x float> %a0, <2 x float> %a1, <4 x i32> poison
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: ret <4 x float> %s
+;
+  %s = shufflevector <2 x float> %a0, <2 x float> %a1, <4 x i32> poison
+  ret <4 x float> %s
+}

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Simon Pilgrim (RKSimon)

Changes

#145920 exposed an issue where we were treating undef/poison shuffles as SK_Select kinds

---

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

2 Files Affected:

• (modified) llvm/include/llvm/Analysis/TargetTransformInfoImpl.h (+4)
• (modified) llvm/test/Analysis/CostModel/X86/alternate-shuffle-cost.ll (+19)

diff --git a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
index d513e9472a152..12f87226c5f57 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
@@ -1534,6 +1534,10 @@ class TargetTransformInfoImplCRTPBase : public TargetTransformInfoImplBase {
       ArrayRef<int> Mask = Shuffle->getShuffleMask();
       int NumSubElts, SubIndex;
 
+      // Treat undef/poison mask as free (no matter the length).
+      if (all_of(Mask, [](int M) { return M < 0; }))
+        return TTI::TCC_Free;
+
       // TODO: move more of this inside improveShuffleKindFromMask.
       if (Shuffle->changesLength()) {
         // Treat a 'subvector widening' as a free shuffle.
diff --git a/llvm/test/Analysis/CostModel/X86/alternate-shuffle-cost.ll b/llvm/test/Analysis/CostModel/X86/alternate-shuffle-cost.ll
index dc770f4ff74c8..777fb5b2f57d4 100644
--- a/llvm/test/Analysis/CostModel/X86/alternate-shuffle-cost.ll
+++ b/llvm/test/Analysis/CostModel/X86/alternate-shuffle-cost.ll
@@ -758,3 +758,22 @@ define <32 x i8> @test_v32i8_3(<32 x i8> %a, <32 x i8> %b) {
   %1 = shufflevector <32 x i8> %a, <32 x i8> %b, <32 x i32> <i32 0, i32 1, i32 2, i32 3, i32 36, i32 5, i32 38, i32 7, i32 40, i32 9, i32 42, i32 11, i32 44, i32 13, i32 46, i32 15, i32 48, i32 17, i32 50, i32 19, i32 52, i32 21, i32 54, i32 23, i32 56, i32 25, i32 58, i32 27, i32 60, i32 61, i32 62, i32 63>
   ret <32 x i8> %1
 }
+
+; Treat all undef/poison shuffle masks as free.
+define <2 x i32> @test_v2i32_poison(<2 x i32> %a0, <2 x i32> %a1) {
+; CHECK-LABEL: 'test_v2i32_poison'
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: %s = shufflevector <2 x i32> %a0, <2 x i32> %a1, <2 x i32> poison
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: ret <2 x i32> %s
+;
+  %s = shufflevector <2 x i32> %a0, <2 x i32> %a1, <2 x i32> poison
+  ret <2 x i32> %s
+}
+
+define <4 x float> @test_v4f32_v2f32_poison(<2 x float> %a0, <2 x float> %a1) {
+; CHECK-LABEL: 'test_v4f32_v2f32_poison'
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: %s = shufflevector <2 x float> %a0, <2 x float> %a1, <4 x i32> poison
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: ret <4 x float> %s
+;
+  %s = shufflevector <2 x float> %a0, <2 x float> %a1, <4 x i32> poison
+  ret <4 x float> %s
+}

[github-actions]

⚠️ undef deprecator found issues in your code. ⚠️

You can test this locally with the following command:

git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' 'HEAD~1' HEAD llvm/include/llvm/Analysis/TargetTransformInfoImpl.h llvm/test/Analysis/CostModel/X86/alternate-shuffle-cost.ll

The following files introduce new uses of undef:

• llvm/test/Analysis/CostModel/X86/alternate-shuffle-cost.ll

Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields undef. You should use poison values for placeholders instead.

In tests, avoid using undef and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.

For example, this is considered a bad practice:

define void @fn() {
  ...
  br i1 undef, ...
}

Please use the following instead:

define void @fn(i1 %cond) {
  ...
  br i1 %cond, ...
}

Please refer to the Undefined Behavior Manual for more information.

[mikaelholmen]

This seem to fix the problems I saw. I've run a bunch of llvm-stress testcases with this fix without seeing any crashes.

Thanks for the quick fix!