[LoopVectorize] Add FNeg instruction support

[craig.topper]

Add full context

[cameron.mcinally]

FYI that I have an incoming patch that will help make this one more concise. Will have a Diff up in the next hour or so...

[RKSimon]

Worth adding the test with current codegen so this patch shows the delta?

[cameron.mcinally]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

4013

Just posted D62521: Add CreateFNegFMF(...) to the IRBuilder. That should allow you to skip the explicit FMF copy.

[cameron.mcinally]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

4013

Oh, I now see that the BinOperator code above is the generic form too. I don't think there are any other unary operators planned, but it's probably best to keep the UnaryOperator code in line with the BinaryOperator code anyway.

[fhahn]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

4003

The duplication with the binary ops seems a little unfortunate to me. The widening is basically the same, except the number of arguments.

What would it take to make it generic to support both binary & unary ops? If we would have a builder function CreateNAryOp(Opcode, ArrayRef<Value*>) the generic version would be quite compact I think. The casts to UnaryOperator (and BinaryOperator) should not be necessary, right? For the copyIRFlags case, we only care if VecOp is an instruction?

llvm/test/Transforms/LoopVectorize/X86/fneg.ll

1 ↗

(On Diff #201613)

Can you make the test independent of X86? I think you can just pass -force-vector-width=4, as we just want to test the widening, not anything cost-modeling related.

[craig.topper]

Move the test. I'll pre-commit it if we're satisfied.

I'll put together a follow up patch to merge the binary and unary operator handling

[craig.topper]

@fhahn does the test look ok to pre-commit now?

[fhahn]

In D62510#1522129, @craig.topper wrote:

@fhahn does the test look ok to pre-commit now?

Personally I think the auto-generated assertions here do not really add much value, as they contain plenty of unrelated stuff and make the test more fragile than necessary. IMO it would be better to focus the check on the relevant bits: ensure we generate load -> fneg -> store in the vector body. But if you think the complete checks are more valuable, please commit the test as is, I do not have too strong feelings about it.

For some reason I missed the changes to LoopVectorizationCostModel::getInstructionCost. For those, a target-specific test would be good. I think most LV tests for that check the cost printed in the debug messages.

[craig.topper]

-Rebase after committing tests.
-Merge in D62534

[craig.topper]

Use range-based for loop as suggested in the other review.

[craig.topper]

Add comment to IRBuilder function.

[fhahn]

LGTM, thanks!

llvm/include/llvm/IR/IRBuilder.h

Value *CreateUnOp(Instruction::UnaryOps Opc,
     if (auto *VC = dyn_cast<Constant>(V))
       return Insert(Folder.CreateUnOp(Opc, VC), Name);
     Instruction *UnOp = UnaryOperator::Create(Opc, V);
     if (isa<FPMathOperator>(UnOp))
       UnOp = setFPAttrs(UnOp, FPMathTag, FMF);
     return Insert(UnOp, Name);
   }
 
+  Value *CreateNAryOp(unsigned Opc, ArrayRef<Value *> Ops,
+                      const Twine &Name = "",
+                      MDNode *FPMathTag = nullptr) {
+    if (Instruction::isBinaryOp(Opc)) {
+      assert(Ops.size() == 2 && "Invalid number of operands!");
+      return CreateBinOp(static_cast<Instruction::BinaryOps>(Opc),
+                         Ops[0], Ops[1], Name, FPMathTag);
+    }
+    if (Instruction::isUnaryOp(Opc)) {
+      assert(Ops.size() == 1 && "Invalid number of operands!");
+      return CreateUnOp(static_cast<Instruction::UnaryOps>(Opc),
+                        Ops[0], Name, FPMathTag);
+    }
+    llvm_unreachable("Unexpected opcode!");
+  }
+
   //===--------------------------------------------------------------------===//
   // Instruction creation methods: Memory Instructions
   //===--------------------------------------------------------------------===//
 
   AllocaInst *CreateAlloca(Type *Ty, unsigned AddrSpace,
                            Value *ArraySize = nullptr, const Twine &Name = "") {
     return Insert(new AllocaInst(Ty, AddrSpace, ArraySize), Name);
   }

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

void InnerLoopVectorizer::widenInstruction(Instruction &I) {
   case Instruction::UDiv:
   case Instruction::SDiv:
   case Instruction::SRem:
   case Instruction::URem:
   case Instruction::Add:
   case Instruction::FAdd:
   case Instruction::Sub:
   case Instruction::FSub:
+  case Instruction::FNeg:
   case Instruction::Mul:
   case Instruction::FMul:
   case Instruction::FDiv:
   case Instruction::FRem:
   case Instruction::Shl:
   case Instruction::LShr:
   case Instruction::AShr:
   case Instruction::And:
   case Instruction::Or:
   case Instruction::Xor: {
-    // Just widen binops.
-    auto *BinOp = cast<BinaryOperator>(&I);
-    setDebugLocFromInst(Builder, BinOp);
+    // Just widen unops and binops.
+    setDebugLocFromInst(Builder, &I);
 
     for (unsigned Part = 0; Part < UF; ++Part) {
-      Value *A = getOrCreateVectorValue(BinOp->getOperand(0), Part);
-      Value *B = getOrCreateVectorValue(BinOp->getOperand(1), Part);
-      Value *V = Builder.CreateBinOp(BinOp->getOpcode(), A, B);
+      SmallVector<Value *, 2> Ops;
+      for (Value *Op : I.operands())
+        Ops.push_back(getOrCreateVectorValue(Op, Part));
+
+      Value *V = Builder.CreateNAryOp(I.getOpcode(), Ops);
 
-      if (BinaryOperator *VecOp = dyn_cast<BinaryOperator>(V))
-        VecOp->copyIRFlags(BinOp);
+      if (auto *VecOp = dyn_cast<Instruction>(V))
+        VecOp->copyIRFlags(&I);
 
       // Use this vector value for all users of the original instruction.
       VectorLoopValueMap.setVectorValue(&I, Part, V);
-      addMetadata(V, BinOp);
+      addMetadata(V, &I);
     }
 
     break;
   }
   case Instruction::Select: {
     // Widen selects.
     // If the selector is loop invariant we can create a select
     // instruction with a scalar condition. Otherwise, use vector-select.
     auto *SE = PSE.getSE();
     bool InvariantCond =
         SE->isLoopInvariant(PSE.getSCEV(I.getOperand(0)), OrigLoop);
     setDebugLocFromInst(Builder, &I);
 
     // The condition can be loop invariant  but still defined inside the
     // loop. This means that we can't just use the original 'cond' value.
     // We have to take the 'vectorized' value and pick the first lane.
     // Instcombine will make this a no-op.
 
     auto *ScalarCond = getOrCreateScalarValue(I.getOperand(0), {0, 0});
 
     for (unsigned Part = 0; Part < UF; ++Part) {
       Value *Cond = getOrCreateVectorValue(I.getOperand(0), Part);
       Value *Op0 = getOrCreateVectorValue(I.getOperand(1), Part);
if (Op2VK == TargetTransformInfo::OK_AnyValue && Legal->isUniform(Op2))
       Op2VK = TargetTransformInfo::OK_UniformValue;
 
     SmallVector<const Value *, 4> Operands(I->operand_values());
     unsigned N = isScalarAfterVectorization(I, VF) ? VF : 1;
     return N * TTI.getArithmeticInstrCost(
                    I->getOpcode(), VectorTy, TargetTransformInfo::OK_AnyValue,
                    Op2VK, TargetTransformInfo::OP_None, Op2VP, Operands);
   }
+  case Instruction::FNeg: {
+    unsigned N = isScalarAfterVectorization(I, VF) ? VF : 1;
+    return N * TTI.getArithmeticInstrCost(
+                   I->getOpcode(), VectorTy, TargetTransformInfo::OK_AnyValue,
+                   TargetTransformInfo::OK_AnyValue,
+                   TargetTransformInfo::OP_None, TargetTransformInfo::OP_None,
+                   I->getOperand(0));
+  }
   case Instruction::Select: {
     SelectInst *SI = cast<SelectInst>(I);
     const SCEV *CondSCEV = SE->getSCEV(SI->getCondition());
     bool ScalarCond = (SE->isLoopInvariant(CondSCEV, TheLoop));
     Type *CondTy = SI->getCondition()->getType();
     if (!ScalarCond)
       CondTy = VectorType::get(CondTy, VF);
 
auto IsVectorizableOpcode = [](unsigned Opcode) {
     case Instruction::AShr:
     case Instruction::BitCast:
     case Instruction::Br:
     case Instruction::Call:
     case Instruction::FAdd:
     case Instruction::FCmp:
     case Instruction::FDiv:
     case Instruction::FMul:
+    case Instruction::FNeg:
     case Instruction::FPExt:
     case Instruction::FPToSI:
     case Instruction::FPToUI:
     case Instruction::FPTrunc:
     case Instruction::FRem:
     case Instruction::FSub:
     case Instruction::GetElementPtr:
     case Instruction::ICmp:

llvm/test/Transforms/LoopVectorize/X86/fneg-cost.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt %s -loop-vectorize -debug-only=loop-vectorize -S 2>&1 | FileCheck %s
 ; REQUIRES: asserts
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.8.0"
 
-; CHECK: Found an estimated cost of 2 for VF 1 For instruction:   %neg = fneg float %{{.*}}
-; CHECK: Found an estimated cost of 6 for VF 2 For instruction:   %neg = fneg float %{{.*}}
-; CHECK: Found an estimated cost of 14 for VF 4 For instruction:   %neg = fneg float %{{.*}}
+; CHECK: Found an estimated cost of 4 for VF 1 For instruction:   %neg = fneg float %{{.*}}
+; CHECK: Found an estimated cost of 4 for VF 2 For instruction:   %neg = fneg float %{{.*}}
+; CHECK: Found an estimated cost of 4 for VF 4 For instruction:   %neg = fneg float %{{.*}}
 define void @fneg_cost(float* %a, i64 %n) {
 entry:
   br label %for.body
 for.body:                                         ; preds = %for.body.preheader, %for.body
   %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
   %arrayidx = getelementptr inbounds float, float* %a, i64 %indvars.iv
   %0 = load float, float* %arrayidx, align 4
   %neg = fneg float %0
   store float %neg, float* %arrayidx, align 4
   %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
   %cmp = icmp eq i64 %indvars.iv.next, %n
   br i1 %cmp, label %for.end, label %for.body
 
 for.end:
   ret void
 }

llvm/test/Transforms/LoopVectorize/fneg.ll

 ; RUN: opt %s -loop-vectorize -force-vector-interleave=1 -force-vector-width=4 -S | FileCheck %s
 
 define void @foo(float* %a, i64 %n) {
 ; CHECK:       vector.body:
 ; CHECK:         [[WIDE_LOAD:%.*]] = load <4 x float>, <4 x float>* {{.*}}, align 4
-; CHECK-NEXT:    [[TMP4:%.*]] = extractelement <4 x float> [[WIDE_LOAD]], i32 0
-; CHECK-NEXT:    [[TMP5:%.*]] = fneg float [[TMP4]]
-; CHECK-NEXT:    [[TMP6:%.*]] = extractelement <4 x float> [[WIDE_LOAD]], i32 1
-; CHECK-NEXT:    [[TMP7:%.*]] = fneg float [[TMP6]]
-; CHECK-NEXT:    [[TMP8:%.*]] = extractelement <4 x float> [[WIDE_LOAD]], i32 2
-; CHECK-NEXT:    [[TMP9:%.*]] = fneg float [[TMP8]]
-; CHECK-NEXT:    [[TMP10:%.*]] = extractelement <4 x float> [[WIDE_LOAD]], i32 3
-; CHECK-NEXT:    [[TMP11:%.*]] = fneg float [[TMP10]]
-; CHECK-NEXT:    [[TMP12:%.*]] = insertelement <4 x float> undef, float [[TMP5]], i32 0
-; CHECK-NEXT:    [[TMP13:%.*]] = insertelement <4 x float> [[TMP12]], float [[TMP7]], i32 1
-; CHECK-NEXT:    [[TMP14:%.*]] = insertelement <4 x float> [[TMP13]], float [[TMP9]], i32 2
-; CHECK-NEXT:    [[TMP15:%.*]] = insertelement <4 x float> [[TMP14]], float [[TMP11]], i32 3
-; CHECK:         store <4 x float> [[TMP15]], <4 x float>* {{.*}}, align 4
+; CHECK-NEXT:    [[TMP4:%.*]] = fneg <4 x float> [[WIDE_LOAD]]
+; CHECK:         store <4 x float> [[TMP4]], <4 x float>* {{.*}}, align 4
 ;
 entry:
   br label %for.body
 
 for.body:
   %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
   %arrayidx = getelementptr inbounds float, float* %a, i64 %indvars.iv
   %0 = load float, float* %arrayidx, align 4