[llvm] [LoopVectorize] LLVM fails to vectorise loops with multi-bool varables (PR #89226)

Sander de Smalen via llvm-commits llvm-commits at lists.llvm.org
Mon May 13 02:49:55 PDT 2024


================
@@ -635,14 +635,57 @@ RecurrenceDescriptor::isAnyOfPattern(Loop *Loop, PHINode *OrigPhi,
       return InstDesc(Select, Prev.getRecKind());
   }
 
+  SelectInst *SI = dyn_cast<SelectInst>(I);
+  Instruction *Cmp = nullptr;
+
+  if (SI) {
+    // Check that SelectInst is related to the this PHI reduction.
+    bool HasOrigPhiUser = false;
+    bool SelectNonPHIUserInLoop = false;
+    for (User *U : SI->users()) {
+      Instruction *Inst = dyn_cast<Instruction>(U);
+      if (!Inst)
+        continue;
+      if (Inst == OrigPhi) {
+        HasOrigPhiUser = true;
+      } else {
+        if (Loop->contains(Inst->getParent()))
+          SelectNonPHIUserInLoop = true;
+      }
+    }
+    Cmp = dyn_cast<CmpInst>(SI->getOperand(0));
+    // Checking the current CmpInst is safe as a recurrent reduction.
+    if (Cmp && !Cmp->hasOneUse() && HasOrigPhiUser && !SelectNonPHIUserInLoop) {
+      bool IsSafeCMP = true;
+      for (User *U : Cmp->users()) {
+        Instruction *UInst = dyn_cast<Instruction>(U);
+        if (!UInst)
+          continue;
+        if (SelectInst *SI1 = dyn_cast<SelectInst>(U)) {
+          if (!llvm::all_of(SI1->users(), [Loop](User *USI) {
+                Instruction *Inst1 = dyn_cast<Instruction>(USI);
+                if (!Inst1 || !Loop->contains(Inst1->getParent()) ||
+                    isa<PHINode>(Inst1))
+                  return true;
+                return false;
+              }))
+            IsSafeCMP = false;
+        }
+        if (IsSafeCMP && !isa<BranchInst>(UInst) && !isa<SelectInst>(UInst) &&
+            Loop->contains(UInst->getParent()))
+          IsSafeCMP = false;
+      }
+      if (!IsSafeCMP)
+        Cmp = nullptr;
+    }
+  }
+
   // Only match select with single use cmp condition.
-  if (!match(I, m_Select(m_OneUse(m_Cmp(Pred, m_Value(), m_Value())), m_Value(),
-                         m_Value())))
----------------
sdesmalen-arm wrote:

When I ignore the code changes in this PR and simply remove the `m_OneUse` in this line of code, then all the tests added in this PR still pass. The only test failure I see is in the existing test `llvm/test/Transforms/LoopVectorize/select-cmp.ll`, where I think the vectorized result is not incorrect. I'm not sure if that would be different if the resulting PHI node (that uses the zero-extended result of the `icmp`) is used, which it currently isn't.

In either case, the lack of test failures suggests to me that the complicated logic added in this patch isn't really being tested by any of the tests?

https://github.com/llvm/llvm-project/pull/89226


More information about the llvm-commits mailing list