[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:
> ABataev wrote:
> > 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.
> > only for stores but not for fadd, fneg
> 
> Is that because you are targeting stores only in this patch? How does it affect other operations?
> It is not yet clear for me why you would like to avoid that analysis for them.
Yes, I just don't have enough time/resources for other ops analysis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132750