[APFloat] Remove neg in favour of unary operator-

[ekatz]

This is a simple change, so generally it looks good, but I have to ask:
What is the reason for it? I do understand why you want to add functionality (the operator- in the other change), but I am not sure why to remove.

[foad]

In D75511#1902991, @ekatz wrote:

What is the reason for it? I do understand why you want to add functionality (the operator- in the other change), but I am not sure why to remove.

Just the general principle of cleaning up after yourself. I think it's silly to have two different names for a simple negation function. If you only ever add functionality, and never remove the old obsoleted functionality, you'll end up with a lot of useless old cruft in your codebase.

[sepavloff]

In D75511#1903158, @foad wrote:

In D75511#1902991, @ekatz wrote:

What is the reason for it? I do understand why you want to add functionality (the operator- in the other change), but I am not sure why to remove.

Just the general principle of cleaning up after yourself. I think it's silly to have two different names for a simple negation function. If you only ever add functionality, and never remove the old obsoleted functionality, you'll end up with a lot of useless old cruft in your codebase.

Some C++ programming styles discourage using overloaded operators, because functions express programmer's intentions clearer.

[foad]

In D75511#1903227, @sepavloff wrote:

Some C++ programming styles discourage using overloaded operators, because functions express programmer's intentions clearer.

Sure, but I don't see anything in the LLVM Programmer’s Manual or LLVM Coding Standards that discourages it. And I think overloading arithmetic operators for a numeric type is the sanest possible use of overloading.

[ekatz]

It is a matter of taste. I actually see the benefits of the clear code when using operator overloads.
The question is, should we remove the explicit function calls?
Lets say we do; I still prefer (above all) a unified coding style. In this case, it interprets as: removing all the other operators. The problem is, with add (for example). It receives a roundingMode as a parameter, which means that we can't remove this function, in exchange to the operator+. We must maintain both.
So, to maintain consistency, I think we should keep neg as well.

[foad]

Update users of neg to use operator- instead.

llvm/include/llvm/ADT/APFloat.h

inline APFloat frexp(const APFloat &X, int &Exp, APFloat::roundingMode RM) {
   llvm_unreachable("Unexpected semantics");
 }
 /// Returns the absolute value of the argument.
 inline APFloat abs(APFloat X) {
   X.clearSign();
   return X;
 }
 
-/// Returns the negated value of the argument.
-inline APFloat neg(APFloat X) {
-  X.changeSign();
-  return X;
-}
-
 /// Implements IEEE minNum semantics. Returns the smaller of the 2 arguments if
 /// both are not NaN. If either argument is a NaN, returns the other argument.
 LLVM_READONLY
 inline APFloat minnum(const APFloat &A, const APFloat &B) {
   if (A.isNaN())
     return B;
   if (B.isNaN())
     return A;

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

TargetLowering::getNegatibleCost(SDValue Op, SelectionDAG &DAG,
   switch (Op.getOpcode()) {
   case ISD::ConstantFP: {
     if (!LegalOperations)
       return NegatibleCost::Neutral;
 
     // Don't invert constant FP values after legalization unless the target says
     // the negated constant is legal.
     if (isOperationLegal(ISD::ConstantFP, VT) ||
-        isFPImmLegal(neg(cast<ConstantFPSDNode>(Op)->getValueAPF()), VT,
+        isFPImmLegal(-cast<ConstantFPSDNode>(Op)->getValueAPF(), VT,
                      ForCodeSize))
       return NegatibleCost::Neutral;
     break;
   }
   case ISD::BUILD_VECTOR: {
     // Only permit BUILD_VECTOR of constants.
     if (llvm::any_of(Op->op_values(), [&](SDValue N) {
           return !N.isUndef() && !isa<ConstantFPSDNode>(N);
         }))
       return NegatibleCost::Expensive;
     if (!LegalOperations)
       return NegatibleCost::Neutral;
     if (isOperationLegal(ISD::ConstantFP, VT) &&
         isOperationLegal(ISD::BUILD_VECTOR, VT))
       return NegatibleCost::Neutral;
     if (llvm::all_of(Op->op_values(), [&](SDValue N) {
           return N.isUndef() ||
-                 isFPImmLegal(neg(cast<ConstantFPSDNode>(N)->getValueAPF()), VT,
+                 isFPImmLegal(-cast<ConstantFPSDNode>(N)->getValueAPF(), VT,
                               ForCodeSize);
         }))
       return NegatibleCost::Neutral;
     break;
   }
   case ISD::FADD: {
     if (!Options.NoSignedZerosFPMath && !Flags.hasNoSignedZeros())
       return NegatibleCost::Expensive;

llvm/lib/IR/ConstantFold.cpp

Constant *llvm::ConstantFoldUnaryInstruction(unsigned Opcode, Constant *C) {
   assert(!isa<ConstantInt>(C) && "Unexpected Integer UnaryOp");
 
   if (ConstantFP *CFP = dyn_cast<ConstantFP>(C)) {
     const APFloat &CV = CFP->getValueAPF();
     switch (Opcode) {
     default:
       break;
     case Instruction::FNeg:
-      return ConstantFP::get(C->getContext(), neg(CV));
+      return ConstantFP::get(C->getContext(), -CV);
     }
   } else if (VectorType *VTy = dyn_cast<VectorType>(C->getType())) {
     // Do not iterate on scalable vector. The number of elements is unknown at
     // compile-time.
     if (IsScalableVector)
       return nullptr;
 
     // Fold each element and create a vector constant from those constants.

llvm/unittests/ADT/APFloatTest.cpp

TEST(APFloatTest, neg) {
   APFloat NegOne = APFloat(APFloat::IEEEsingle(), "-1.0");
   APFloat Zero = APFloat::getZero(APFloat::IEEEsingle(), false);
   APFloat NegZero = APFloat::getZero(APFloat::IEEEsingle(), true);
   APFloat Inf = APFloat::getInf(APFloat::IEEEsingle(), false);
   APFloat NegInf = APFloat::getInf(APFloat::IEEEsingle(), true);
   APFloat QNaN = APFloat::getNaN(APFloat::IEEEsingle(), false);
   APFloat NegQNaN = APFloat::getNaN(APFloat::IEEEsingle(), true);
 
-  EXPECT_TRUE(NegOne.bitwiseIsEqual(neg(One)));
-  EXPECT_TRUE(One.bitwiseIsEqual(neg(NegOne)));
-  EXPECT_TRUE(NegZero.bitwiseIsEqual(neg(Zero)));
-  EXPECT_TRUE(Zero.bitwiseIsEqual(neg(NegZero)));
-  EXPECT_TRUE(NegInf.bitwiseIsEqual(neg(Inf)));
-  EXPECT_TRUE(Inf.bitwiseIsEqual(neg(NegInf)));
-  EXPECT_TRUE(NegInf.bitwiseIsEqual(neg(Inf)));
-  EXPECT_TRUE(Inf.bitwiseIsEqual(neg(NegInf)));
-  EXPECT_TRUE(NegQNaN.bitwiseIsEqual(neg(QNaN)));
-  EXPECT_TRUE(QNaN.bitwiseIsEqual(neg(NegQNaN)));
-
   EXPECT_TRUE(NegOne.bitwiseIsEqual(-One));
   EXPECT_TRUE(One.bitwiseIsEqual(-NegOne));
   EXPECT_TRUE(NegZero.bitwiseIsEqual(-Zero));
   EXPECT_TRUE(Zero.bitwiseIsEqual(-NegZero));
   EXPECT_TRUE(NegInf.bitwiseIsEqual(-Inf));
   EXPECT_TRUE(Inf.bitwiseIsEqual(-NegInf));
   EXPECT_TRUE(NegInf.bitwiseIsEqual(-Inf));
   EXPECT_TRUE(Inf.bitwiseIsEqual(-NegInf));