[SLPVectorizer] Ensure dominated reduction values.

[• chatur01]

Summary

When considering incoming values as part of a reduction phi, ensure the
incoming value is dominated by said phi.

Failing to ensure this property causes miscompiles.

Fixes PR25787.

Many thanks to Mattias Eriksson for reporting, reducing and analyzing the
problem for me.

[nadav]

LGTM.

Thanks for fixing this Charlie!

[• chatur01]

Thanks for the review Nadav!

lib/Transforms/Vectorize/SLPVectorizer.cpp

 
 /// \brief Try and get a reduction value from a phi node.
 ///
 /// Given a phi node \p P in a block \p ParentBB, consider possible reductions
 /// if they come from either \p ParentBB or a containing loop latch.
 ///
 /// \returns A candidate reduction value if possible, or \code nullptr \endcode
 /// if not possible.
-static Value *getReductionValue(PHINode *P, BasicBlock *ParentBB,
-                                LoopInfo *LI) {
+static Value *getReductionValue(const DominatorTree *DT, PHINode *P,
+                                BasicBlock *ParentBB, LoopInfo *LI) {
+  // There are situations where the reduction value is not dominated by the
+  // reduction phi. Vectorizing such cases has been reported to cause
+  // miscompiles. See PR25787.
+  auto DominatedReduxValue = [&](Value *R) {
+    return (
+        dyn_cast<Instruction>(R) &&
+        DT->dominates(P->getParent(), dyn_cast<Instruction>(R)->getParent()));
+  };
+
   Value *Rdx = nullptr;
 
   // Return the incoming value if it comes from the same BB as the phi node.
   if (P->getIncomingBlock(0) == ParentBB) {
     Rdx = P->getIncomingValue(0);
   } else if (P->getIncomingBlock(1) == ParentBB) {
     Rdx = P->getIncomingValue(1);
   }
 
-  if (Rdx)
+  if (Rdx && DominatedReduxValue(Rdx))
     return Rdx;
 
   // Otherwise, check whether we have a loop latch to look at.
   Loop *BBL = LI->getLoopFor(ParentBB);
   if (!BBL)
-    return Rdx;
+    return nullptr;
   BasicBlock *BBLatch = BBL->getLoopLatch();
   if (!BBLatch)
-    return Rdx;
+    return nullptr;
 
   // There is a loop latch, return the incoming value if it comes from
   // that. This reduction pattern occassionaly turns up.
   if (P->getIncomingBlock(0) == BBLatch) {
     Rdx = P->getIncomingValue(0);
   } else if (P->getIncomingBlock(1) == BBLatch) {
     Rdx = P->getIncomingValue(1);
   }
 
+  if (Rdx && DominatedReduxValue(Rdx))
     return Rdx;
+
+  return nullptr;
 }
 
 /// \brief Attempt to reduce a horizontal reduction.
 /// If it is legal to match a horizontal reduction feeding
 /// the phi node P with reduction operators BI, then check if it
 /// can be done.
 /// \returns true if a horizontal reduction was matched and reduced.
 /// \returns false if a horizontal reduction was not matched.
if (isa<DbgInfoIntrinsic>(it))
       continue;
 
     // Try to vectorize reductions that use PHINodes.
     if (PHINode *P = dyn_cast<PHINode>(it)) {
       // Check that the PHI is a reduction PHI.
       if (P->getNumIncomingValues() != 2)
         return Changed;
 
-      Value *Rdx = getReductionValue(P, BB, LI);
+      Value *Rdx = getReductionValue(DT, P, BB, LI);
 
       // Check if this is a Binary Operator.
       BinaryOperator *BI = dyn_cast_or_null<BinaryOperator>(Rdx);
       if (!BI)
         continue;
 
       // Try to match and vectorize a horizontal reduction.
       if (canMatchHorizontalReduction(P, BI, R, TTI)) {

test/Transforms/SLPVectorizer/AArch64/pr25787.ll

+; RUN: opt -slp-vectorizer -slp-threshold=-100 < %s
+
+; This testcase ensures a that the reduction values feeding a phi
+; are dominated by said phi. Failing to handle this case can cause
+; codegen errors.
+; This test case will crash if such an attempt is made to vectorize
+; such a reduction.
+
+target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64--linux"
+
+define void @pr25787() {
+bb4:
+  %_tmp473 = load volatile i16, i16* undef, align 1
+  %_tmp503 = load volatile i16, i16* undef, align 1
+  %_tmp505 = load volatile i16, i16* undef, align 1
+  %_tmp519 = load volatile i16, i16* undef, align 1
+  %_tmp521 = load volatile i16, i16* undef, align 1
+  %_tmp527 = load volatile i16, i16* undef, align 1
+  %_tmp529 = load volatile i16, i16* undef, align 1
+  %_tmp535 = load volatile i16, i16* undef, align 1
+  %_tmp976 = sub i16 %_tmp527, %_tmp521
+  %_tmp983 = sub i16 %_tmp535, %_tmp529
+  %_tmp1088 = sub i16 %_tmp503, %_tmp473
+  %_tmp1091 = add i16 %_tmp983, %_tmp976
+  %_tmp1094 = sub i16 %_tmp519, %_tmp505
+  %_tmp1095 = add i16 %_tmp1091, %_tmp1094
+  %_tmp1119 = add i16 %_tmp1095, %_tmp1088
+  br label %bb15
+
+bb15:                                             ; preds = %bb4, %bb15
+  %f7.355.0 = phi i16 [ 0, %bb4 ], [ %_tmp1119, %bb15 ]
+  br label %bb15
+}
+