[IRBuilder] Add CreateFNegFMF(...) to the IRBuilder

[cameron.mcinally]

Summary

@craig.topper brought up the question -- do we want to call UnaryOperator::CreateFNeg(...) or BinaryOperator::CreateFNeg(...) here. My current thinking is that we want to call the UnaryOperator flavor, since this is a new entry point. Using the UnaryOperator is the end goal and there will be no regressions from this decision. This will minimize the work going forward.

I'm open to other opinions though...

[craig.topper]

So my thought is that using UnaryOperator::CreateFNeg is only safe if we only use it when we started with an fneg. Or that the we are sure all passes downstream of whichever passes uses this can handle the presence of an fneg instruction in IR even if they don't know how to optimize it. Basically I'm concerned with sudden introduction of fnegs into IR before all passes are ready. But if we started with an fneg then it must have been testing IR and not clang produced IR.

[cameron.mcinally]

In D62521#1521599, @craig.topper wrote:

So my thought is that using UnaryOperator::CreateFNeg is only safe if we only use it when we started with an fneg. Or that the we are sure all passes downstream of whichever passes uses this can handle the presence of an fneg instruction in IR even if they don't know how to optimize it. Basically I'm concerned with sudden introduction of fnegs into IR before all passes are ready. But if we started with an fneg then it must have been testing IR and not clang produced IR.

That is a good point.

So we can't say, right now, that producing the unary FNeg is relatively optimal, but I would think that it's fairly functional from the existence of ISD::FNEG in ISel. If targets didn't support that, we'd see failures already (or they're untested).

I guess my concern is that producing a binary FNeg in this new function pushes the goal posts back. New uses of this function may need to be optimized later, which may be even more work. We also know that there will be no regressions, so why not move forward.

That said, I could see the argument that CreateFNeg(...) + separate code to copy FMF does not function the same as CreateFNegFMF(...).

I don't feel particularly strongly about this either way though...

[cameron.mcinally]

This comment was removed by cameron.mcinally.

[cameron.mcinally]

Have IRBuilder::CreateFNegFMF(...) return a BinaryOperator as discussed...

[cameron.mcinally]

Ping...

[spatel]

LGTM

llvm/include/llvm/IR/IRBuilder.h

1381–1382 ↗

(On Diff #202426)

Add a TODO comment about using UnaryOperator here in the future, so we don't forget?

llvm/trunk/include/llvm/IR/IRBuilder.h

public:
   }
 
   Value *CreateNot(Value *V, const Twine &Name = "") {
     if (auto *VC = dyn_cast<Constant>(V))
       return Insert(Folder.CreateNot(VC), Name);
     return Insert(BinaryOperator::CreateNot(V), Name);
   }
 
+  /// Copy fast-math-flags from an instruction rather than using the builder's
+  /// default FMF.
+  Value *CreateFNegFMF(Value *V, Instruction *FMFSource,
+                       const Twine &Name = "") {
+   if (auto *VC = dyn_cast<Constant>(V))
+     return Insert(Folder.CreateFNeg(VC), Name);
+   // TODO: This should return UnaryOperator::CreateFNeg(...) once we are
+   // confident that they are optimized sufficiently.
+   return Insert(setFPAttrs(BinaryOperator::CreateFNeg(V), nullptr,
+                            FMFSource->getFastMathFlags()),
+                 Name);
+  }
+
   Value *CreateUnOp(Instruction::UnaryOps Opc,
                     Value *V, const Twine &Name = "",
                     MDNode *FPMathTag = nullptr) {
     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);

llvm/trunk/unittests/IR/IRBuilderTest.cpp

TEST_F(IRBuilderTest, GetIntTy) {
   EXPECT_EQ(IntPtrTy, IntegerType::get(Ctx, IntPtrBitSize));
   delete DL;
 }
 
 TEST_F(IRBuilderTest, UnaryOperators) {
   IRBuilder<NoFolder> Builder(BB);
   Value *V = Builder.CreateLoad(GV->getValueType(), GV);
 
-  // Test CreateUnOp
+  // Test CreateUnOp(X)
   Value *U = Builder.CreateUnOp(Instruction::FNeg, V);
   ASSERT_TRUE(isa<Instruction>(U));
   ASSERT_TRUE(isa<FPMathOperator>(U));
   ASSERT_TRUE(isa<UnaryOperator>(U));
   ASSERT_FALSE(isa<BinaryOperator>(U));
+
+  // Test CreateFNegFMF(X)
+  Instruction *I = cast<Instruction>(V);
+  I->setHasNoSignedZeros(true);
+  I->setHasNoNaNs(true);
+  Value *VFMF = Builder.CreateFNegFMF(V, I);
+  Instruction *IFMF = cast<Instruction>(VFMF);
+  EXPECT_TRUE(IFMF->hasNoSignedZeros());
+  EXPECT_TRUE(IFMF->hasNoNaNs());
+  EXPECT_FALSE(IFMF->hasAllowReassoc());
 }
 
 TEST_F(IRBuilderTest, FastMathFlags) {
   IRBuilder<> Builder(BB);
   Value *F, *FC;
   Instruction *FDiv, *FAdd, *FCmp, *FCall;
 
   F = Builder.CreateLoad(GV->getValueType(), GV);