[SLP]Fix PR57322: vectorize constant float stores.

[ABataev]

Summary

Stores for constant floats must be vectorized, improve analysis in SLP
vectorizer for stores.

[vdmitrie]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

5819

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};

[vdmitrie]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

5819

Minor correction for the above (VP Initialization):

TTI::OperandValueProperties VP =
    CInt0->getValue().isPowerOf2() ? TTI::OP_PowerOf2 : TTI::OP_None;

[ABataev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

5819

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.

[vdmitrie]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

5819

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.

[ABataev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

5819

Yes, I just don't have enough time/resources for other ops analysis.

[vdmitrie]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

5819

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.

[ABataev]

Address comments

[vdmitrie]

Looks acceptable.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

5835

As I said earlier we should never have a situation of mixed types. So I do not quite understand why you are still sticking with packing the two flows into the single loop. This does not save anything but makes the code to be error prone. Just look: the condition at 5840 should probably be if (EnableFP && CFlt). You also do not use CFlt to access the object but only for null pointer check.
Technically yes, this code will work. But I appreciate if you address maintenance concern as well.

[ABataev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

5835

Sure, just missed it.

[reames]

Adding this only for the store case seems undesirable both long term as it's clearly not the right long term goal and short term as it adds complexity for no good reason. I'm not actively requesting you don't land this, but I do strongly want to encourage you to either a) do the analysis for the non-stores and refresh this patch, or b) commit to doing a follow up in the near future.

[ABataev]

In D132750#3756195, @reames wrote:

Adding this only for the store case seems undesirable both long term as it's clearly not the right long term goal and short term as it adds complexity for no good reason. I'm not actively requesting you don't land this, but I do strongly want to encourage you to either a) do the analysis for the non-stores and refresh this patch, or b) commit to doing a follow up in the near future.

I can try to add OK_FPConstantValue value instead and change analysis using this value for now.

[reames]

In D132750#3756281, @ABataev wrote:

In D132750#3756195, @reames wrote:

Adding this only for the store case seems undesirable both long term as it's clearly not the right long term goal and short term as it adds complexity for no good reason. I'm not actively requesting you don't land this, but I do strongly want to encourage you to either a) do the analysis for the non-stores and refresh this patch, or b) commit to doing a follow up in the near future.

I can try to add OK_FPConstantValue value instead and change analysis using this value for now.

Huh? Why do this?

I meant that we clearly want uniform constant handling to include float vectors in all cases long term. That's what I was asking you to follow up with.

[ABataev]

In D132750#3756310, @reames wrote:

In D132750#3756281, @ABataev wrote:

In D132750#3756195, @reames wrote:

Adding this only for the store case seems undesirable both long term as it's clearly not the right long term goal and short term as it adds complexity for no good reason. I'm not actively requesting you don't land this, but I do strongly want to encourage you to either a) do the analysis for the non-stores and refresh this patch, or b) commit to doing a follow up in the near future.

I can try to add OK_FPConstantValue value instead and change analysis using this value for now.

Huh? Why do this?

I meant that we clearly want uniform constant handling to include float vectors in all cases long term. That's what I was asking you to follow up with.

I see. I can send a followup patch with uniform floats analysis but I cannot test it thoroughly.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

private:
   }
 
   /// Checks if all users of \p I are the part of the vectorization tree.
   bool areAllUsersVectorized(Instruction *I,
                              ArrayRef<Value *> VectorizedVals) const;
 
   /// Return information about the vector formed for the specified index
   /// of a vector of (the same) instruction.
+  /// \param WithFloat - If true, check for float constants.
   TargetTransformInfo::OperandValueInfo
-  getOperandInfo(ArrayRef<Value *> VL, unsigned OpIdx);
+  getOperandInfo(ArrayRef<Value *> VL, unsigned OpIdx, bool WithFloat = false);
 
   /// \returns the cost of the vectorizable entry.
   InstructionCost getEntryCost(const TreeEntry *E,
                                ArrayRef<Value *> VectorizedVals);
 
   /// This is the recursive part of buildTree.
   void buildTree_rec(ArrayRef<Value *> Roots, unsigned Depth,
                      const EdgeInfo &EI);
if (P0 == AltP0Swapped)
              (I != MainOp &&
               !areCompatibleCmpOps(CI0->getOperand(0), CI0->getOperand(1),
                                    CI->getOperand(0), CI->getOperand(1)));
     return AltP0 == CurrentPred || AltP0Swapped == CurrentPred;
   }
   return I->getOpcode() == AltOp->getOpcode();
 }
 
-TTI::OperandValueInfo BoUpSLP::getOperandInfo(ArrayRef<Value *> VL, unsigned OpIdx) {
-
+TTI::OperandValueInfo BoUpSLP::getOperandInfo(ArrayRef<Value *> VL,
+                                              unsigned OpIdx, bool WithFloat) {
   TTI::OperandValueKind VK = TTI::OK_UniformConstantValue;
   TTI::OperandValueProperties VP = TTI::OP_PowerOf2;
 
   // 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 = nullptr;
-  for (unsigned i = 0, e = VL.size(); i < e; ++i) {
-    const Instruction *I = cast<Instruction>(VL[i]);
-    assert(I->getOpcode() == cast<Instruction>(VL[0])->getOpcode());
-    ConstantInt *CInt = dyn_cast<ConstantInt>(I->getOperand(OpIdx));
-    if (!CInt) {
+  for (auto [I, V] : enumerate(VL)) {
+    const auto *Inst = cast<Instruction>(V);
+    assert(Inst->getOpcode() == cast<Instruction>(VL[0])->getOpcode() &&
+           "Expected same opcode");
+    auto *CInt = dyn_cast<ConstantInt>(Inst->getOperand(OpIdx));
+    auto *CFlt = dyn_cast<ConstantFP>(Inst->getOperand(OpIdx));
+    if (!CInt && (!WithFloat || !CFlt)) {
       VK = TTI::OK_AnyValue;
       VP = TTI::OP_None;
       break;
     }
-    if (VP == TTI::OP_PowerOf2 &&
-        !CInt->getValue().isPowerOf2())
+    if (CFlt) {
+      VK = TTI::OK_NonUniformConstantValue;
+      VP = TTI::OP_None;
+      continue;
+    }
+    if (VP == TTI::OP_PowerOf2 && !CInt->getValue().isPowerOf2())
       VP = TTI::OP_None;
-    if (i == 0) {
+    if (I == 0) {
       CInt0 = CInt;
       continue;
     }
     if (CInt0 != CInt)
       VK = TTI::OK_NonUniformConstantValue;
   }
   return {VK, VP};
 }
case Instruction::Load: {
       return CommonCost + VecLdCost - ScalarLdCost;
     }
     case Instruction::Store: {
       // We know that we can merge the stores. Calculate the cost.
       bool IsReorder = !E->ReorderIndices.empty();
       auto *SI =
           cast<StoreInst>(IsReorder ? VL[E->ReorderIndices.front()] : VL0);
       Align Alignment = SI->getAlign();
-      TTI::OperandValueInfo OpInfo = TTI::getOperandInfo(SI->getOperand(0));
+      TTI::OperandValueInfo OpInfo = getOperandInfo(VL, 0, /*WithFloat=*/true);
       InstructionCost ScalarEltCost = TTI->getMemoryOpCost(
           Instruction::Store, ScalarTy, Alignment, 0, CostKind, OpInfo, VL0);
       InstructionCost ScalarStCost = VecTy->getNumElements() * ScalarEltCost;
       TTI::OperandValueKind OpVK = TTI::OK_AnyValue;
-      if (all_of(E->Scalars,
-                 [](Value *V) {
-                   return isConstant(cast<Instruction>(V)->getOperand(0));
-                 }) &&
-          any_of(E->Scalars, [](Value *V) {
-            Value *Op = cast<Instruction>(V)->getOperand(0);
-            return !isa<UndefValue>(Op) && !cast<Constant>(Op)->isZeroValue();
-          }))
+      if (OpInfo.isConstant())
         OpVK = TTI::OK_NonUniformConstantValue;
       InstructionCost VecStCost = TTI->getMemoryOpCost(
           Instruction::Store, VecTy, Alignment, 0, CostKind,
           {OpVK, TTI::OP_None}, VL0);
       LLVM_DEBUG(dumpTreeCosts(E, CommonCost, VecStCost, ScalarStCost));
       return CommonCost + VecStCost - ScalarStCost;
     }
     case Instruction::Call: {

llvm/test/Transforms/SLPVectorizer/X86/stores_constant_float.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -mtriple=x86_64-unknown-linux-gnu -slp-vectorizer -mcpu=skylake -S -o - %s | FileCheck %s
 
 define void @foo() {
 ; CHECK-LABEL: @foo(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[C:%.*]] = alloca { double, double }, align 8
-; CHECK-NEXT:    [[C_IMAGP:%.*]] = getelementptr inbounds { double, double }, ptr [[C]], i64 0, i32 1
-; CHECK-NEXT:    store double 0.000000e+00, ptr [[C]], align 8
-; CHECK-NEXT:    store double 1.000000e+00, ptr [[C_IMAGP]], align 8
+; CHECK-NEXT:    store <2 x double> <double 0.000000e+00, double 1.000000e+00>, ptr [[C]], align 8
 ; CHECK-NEXT:    ret void
 ;
 entry:
   %c = alloca { double, double }, align 8
   %c.imagp = getelementptr inbounds { double, double }, ptr %c, i64 0, i32 1
   store double 0.000000e+00, ptr %c, align 8
   store double 1.000000e+00, ptr %c.imagp, align 8
   ret void
 }