[SLP][REVEC] Ignore UserTreeIndex if it is empty.

[HanKuanChen]

Previously, the all_of check did not consider the case where the
TreeEntry is empty (i.e., when it is the first entry).

[llvmbot]

@llvm/pr-subscribers-vectorizers

@llvm/pr-subscribers-llvm-transforms

Author: Han-Kuan Chen (HanKuanChen)

Changes

Previously, the all_of check did not consider the case where the
TreeEntry is empty (i.e., when it is the first entry).

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp (+4-3)
• (added) llvm/test/Transforms/SLPVectorizer/X86/revec-reduced-value-replace-extractelement.ll (+35)

diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index a2200f283168d..084da5607e400 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -6449,9 +6449,10 @@ void BoUpSLP::reorderTopToBottom() {
             assert(SLPReVec && "Only supported by REVEC.");
             // ShuffleVectorInst does not do reorderOperands (and it should not
             // because ShuffleVectorInst supports only a limited set of
-            // patterns). Only do reorderNodeWithReuses if all of the users are
-            // not ShuffleVectorInst.
-            if (isa<ShuffleVectorInst>(TE->UserTreeIndex.UserTE->getMainOp()))
+            // patterns). Only do reorderNodeWithReuses if the user is not
+            // ShuffleVectorInst.
+            if (TE->UserTreeIndex &&
+                isa<ShuffleVectorInst>(TE->UserTreeIndex.UserTE->getMainOp()))
               continue;
             assert((!TE->UserTreeIndex ||
                     !isa<ShuffleVectorInst>(
diff --git a/llvm/test/Transforms/SLPVectorizer/X86/revec-reduced-value-replace-extractelement.ll b/llvm/test/Transforms/SLPVectorizer/X86/revec-reduced-value-replace-extractelement.ll
new file mode 100644
index 0000000000000..bdd5971c0c91a
--- /dev/null
+++ b/llvm/test/Transforms/SLPVectorizer/X86/revec-reduced-value-replace-extractelement.ll
@@ -0,0 +1,35 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S --passes=slp-vectorizer -slp-threshold=-99999 < %s -mtriple=x86_64-unknown-linux-gnu -slp-revec | FileCheck %s
+
+define void @test() {
+; CHECK-LABEL: define void @test() {
+; CHECK-NEXT:  [[BB:.*]]:
+; CHECK-NEXT:    [[TRUNC:%.*]] = trunc i64 0 to i32
+; CHECK-NEXT:    br label %[[BB1:.*]]
+; CHECK:       [[BB1]]:
+; CHECK-NEXT:    [[TMP0:%.*]] = phi <2 x i32> [ zeroinitializer, %[[BB]] ], [ [[TMP4:%.*]], %[[BB1]] ]
+; CHECK-NEXT:    [[TMP1:%.*]] = extractelement <2 x i32> [[TMP0]], i32 1
+; CHECK-NEXT:    [[TMP2:%.*]] = call i8 @llvm.vector.reduce.mul.v4i8(<4 x i8> zeroinitializer)
+; CHECK-NEXT:    [[TMP3:%.*]] = zext i8 [[TMP2]] to i32
+; CHECK-NEXT:    [[OP_RDX:%.*]] = mul i32 [[TMP3]], [[TMP1]]
+; CHECK-NEXT:    [[OP_RDX1:%.*]] = mul i32 [[OP_RDX]], [[TRUNC]]
+; CHECK-NEXT:    [[TMP4]] = insertelement <2 x i32> <i32 0, i32 poison>, i32 [[OP_RDX1]], i32 1
+; CHECK-NEXT:    br label %[[BB1]]
+;
+bb:
+  br label %bb1
+
+bb1:
+  %phi = phi i32 [ 0, %bb ], [ %mul9, %bb1 ]
+  %phi2 = phi i32 [ 0, %bb ], [ 0, %bb1 ]
+  %trunc = trunc i64 0 to i32
+  %mul = mul i32 0, %trunc
+  %mul3 = mul i32 %trunc, %phi
+  %mul4 = mul i32 %mul3, %mul
+  %mul5 = mul i32 %mul4, %mul
+  %trunc6 = trunc i64 0 to i32
+  %mul7 = mul i32 0, %trunc6
+  %mul8 = mul i32 %mul5, %mul7
+  %mul9 = mul i32 %mul8, %mul7
+  br label %bb1
+}

[alexey-bataev]

Some of the user may not have state, need to add a check for hasState() before asking for getMainOp

[HanKuanChen]

How? If a TreeEntry has a user, that means the user must have MainOp inside InstructionsState. Otherwise, buildTree_rec cannot work.

[alexey-bataev]

Some entries might have Gather parents

[ilovepi]

We're seeing what appears to be an MSan test failure after this patch on Linux Aarch64 bots. The patch seems pretty benign to me, but this is the only change in the blame list. Any idea about why that could be?

CC: @vitalybuka in case he knows of something else that could be happening.

Bot link: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-arm64/b8719911097381318689/infra

[ilovepi]

We're seeing what appears to be an MSan test failure after this patch on Linux Aarch64 bots. The patch seems pretty benign to me, but this is the only change in the blame list. Any idea about why that could be?

CC: @vitalybuka in case he knows of something else that could be happening.

Bot link: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-arm64/b8719911097381318689/infra

Ugh, sorry for the noise. A team member pointed out that something changed in our infra around that time, so indeed its unrelated to your patch. Apologies.