[PATCH] D132750: [SLP]Fix PR57322: vectorize constant float stores.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5819
+TTI::OperandValueInfo BoUpSLP::getOperandInfo(ArrayRef<Value *> VL,
+                                              unsigned OpIdx, bool WithFloat) {
   TTI::OperandValueKind VK = TTI::OK_UniformConstantValue;
----------------
vdmitrie wrote:
> vdmitrie wrote:
> > I'm not sure I understand why we may need this extra argument. 
> > Is it for restraining purpose?
> > We can tell whether we dealing with FP or integer from type of the operand we are evaluating.
> > Besides, type of operation across VL have to be homogeneous so it makes sense to multi-version the method  for better clarity.
> > And you don't need to enumerate VL if you process first element early.
> > 
> > Here is how the code could look:
> > ```
> >    const auto *I0 = cast<Instruction>(VL[0]);
> >    Type *OpTy = I0->getOperand(OpIdx)->getType();
> >    if (OpTy->isFloatingPointTy()) {
> >      for (Value *V : VL) {
> >        const auto *Inst = cast<Instruction>(V);
> >        assert(Inst->getOpcode() == I0->getOpcode() && "Expected same opcode");
> >        if (!isa<ConstantFP>(Inst->getOperand(OpIdx)))
> >          return {TTI::OK_AnyValue, TTI::OP_None};
> >      }
> >      return {TTI::OK_NonUniformConstantValue, TTI::OP_None};
> >    }
> > 
> >    // If all operands are exactly the same ConstantInt then set the
> >    // operand kind to OK_UniformConstantValue.
> >    // If instead not all operands are constants, then set the operand kind
> >    // to OK_AnyValue. If all operands are constants but not the same,
> >    // then set the operand kind to OK_NonUniformConstantValue.
> >    ConstantInt *CInt0 = dyn_cast<ConstantInt>(I0->getOperand(OpIdx));
> >    if (!CInt0)
> >      return {TTI::OK_AnyValue, TTI::OP_None};
> > 
> >    TTI::OperandValueKind VK = TTI::OK_UniformConstantValue;
> >    TTI::OperandValueProperties VP = TTI::OP_PowerOf2;
> >    for (Value *V : VL.drop_front()) {
> >      const auto *Inst = cast<Instruction>(V);
> >      assert(Inst->getOpcode() == I0->getOpcode() && "Expected same opcode");
> >      auto *CInt = dyn_cast<ConstantInt>(Inst->getOperand(OpIdx));
> >      if (!CInt)
> >        return {TTI::OK_AnyValue, TTI::OP_None};
> >      if (VP == TTI::OP_PowerOf2 && !CInt->getValue().isPowerOf2())
> >        VP = TTI::OP_None;
> >      if (CInt0 != CInt)
> >        VK = TTI::OK_NonUniformConstantValue;
> >    }
> >    return {VK, VP};
> > 
> > ```
> Minor correction for the above (VP Initialization):
>    TTI::OperandValueProperties VP =
>        CInt0->getValue().isPowerOf2() ? TTI::OP_PowerOf2 : TTI::OP_None;
> 
Just we need to perform this abalysis only for stores but not for fadd, fneg, etc. I can add a check fir the kind of the instruction instead but I thought that it would be better to provide a flag explicitly and pass it explicitly for the required instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132750