[PATCH] D70148: [SLP] fix miscompile on min/max reductions with extra uses (PR43948)

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 2 inline comments as done.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6744-6753
+      Instruction *RdxCmp, *Cmp;
+      if (match(cast<Instruction>(ReductionRoot),
+                m_Select(m_Instruction(RdxCmp), m_Value(), m_Value())) &&
+          (RdxCmp->getOpcode() == Instruction::ICmp ||
+           RdxCmp->getOpcode() == Instruction::FCmp) &&
+          match(VectorizedTree,
+                m_Select(m_Instruction(Cmp), m_Value(), m_Value())) &&
----------------
ABataev wrote:
> I don't think you need matches here. You can rely on `ReductionData.getKind()` and then just do something like this:
> ```
> switch (ReductionData.getKind()) {
> case RK_Min:
> case RK_Max:
> case RK_UMin:
> case RK_UMax:
>   cast<SelectInstruction>(ReductionRoot)->getCondition()->replaceAllUsesWith(cast<SelectInstruction>(VectorizedTree)->getCondition());
>   break;
> case RK_Arithmetic:
>   break;
> }
> ```
> And even better to create a new member function in Operation data, which will replace all uses for the vectorized instruction like in this code + the final replacement.
This doesn't work because we don't know that the vectorized code ends in a select at this point (it might end in an extractelement of a vector instruction instead).

But I agree that we can simplify the logic a bit, so added a helper:
rGe9bf7a60a036

I also looked at extending the OperationData class as suggested, but I don't see how to do that cleanly because the final element of the reduction (the thing that we want to substitute for the scalar op) isn't part of OperationData currently. Let's make that a follow-up cleanup step.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70148/new/

https://reviews.llvm.org/D70148