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

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie 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;
----------------
ABataev wrote:
> 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.
That's okay for incremental step.  Then please don't declare the default value for the argument and
please add a TODO comment at the call site where it is 'false' saying that impact of enabling the analysis there is yet to be determined. Also renaming the argument to enableFP or likewise would make restraining purpose more explicit.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132750