[APFloat] Overload unary operator-

[foad]

Summary

We already have overloaded binary arithemetic operators so you can write
A+B etc. This patch lets you write -A instead of neg(A).

[RKSimon]

llvm/unittests/ADT/APFloatTest.cpp

3001

Don't we need to keep the neg tests as well as adding the unary tests?

[foad]

Test neg(A) as well as -A.

[foad]

llvm/unittests/ADT/APFloatTest.cpp

3001

Fair enough, done, though I planned to remove APFloat::neg in a follow-up.

[ekatz]

llvm/include/llvm/ADT/APFloat.h

1253

Why not make this a member of APFloat, just like the other operator overloads?

llvm/lib/IR/ConstantFold.cpp

989 ↗

(On Diff #246916)

Why change it? Calling operator-() seems a bit more elusive, than the neg.
(This also regards to the changes in "TargetLowering.cpp".)

[foad]

llvm/include/llvm/ADT/APFloat.h

1253

Sure, I could do, but I don't see a compelling reason to do that and I thought it was simpler to follow the example of existing neg.

Others would advise me to make the other operator overloads non-members instead. For example https://en.cppreference.com/w/cpp/language/operators says "Binary operators are typically implemented as non-members [...]".

llvm/lib/IR/ConstantFold.cpp

989 ↗

(On Diff #246916)

I think that's a matter of taste. If I didn't prefer -CV to neg(CV) then I wouldn't have bothered to implement operator-!

[ekatz]

llvm/include/llvm/ADT/APFloat.h

1253

You are correct, placing it after the neg does make sense, and the quoted advise makes much sense as well.
However, in this case, not-making it a member function, creates inconsistency with the other operators, and especially, it looks confusing and strange to have 2 versions of the operator- that take a single argument: one as a member function and the other as non-member.

llvm/lib/IR/ConstantFold.cpp

989 ↗

(On Diff #246916)

I agree, but in this case I would rather not change it, and maybe just use it for future uses.
In particular, it makes more sense to keep using neg in this case, as it explicitly shows the connection to the FNeg enum.
In addition, it is consistent with the FAdd (at line 1338), FSub, FMul, etc. as they are implemented using the explicit function and not the corresponding operator.

[foad]

Move unary operator- next to other overloads.

[foad]

llvm/include/llvm/ADT/APFloat.h

1253

OK, done.

llvm/lib/IR/ConstantFold.cpp

989 ↗

(On Diff #246916)

I'd prefer to remove neg altogether; see D75511. I'm keeping that as a separate patch to avoid a "flag day" change.

I'd prefer to simplify the code at line 1338 to use overloaded operators too. There is still a case for keeping add() etc as functions, because they take an additional rounding mode argument. There is no reason to keep neg() as it is very simple and takes no additional arguments.

[RKSimon]

llvm/lib/IR/ConstantFold.cpp

989 ↗

(On Diff #246916)

In which case I'd recommend moving these changes of neg to the operator into D75511 and this patch just being about adding the operator.

[foad]

llvm/lib/IR/ConstantFold.cpp

989 ↗

(On Diff #246916)

Sure, can do, but I'm wondering why? What if someone asks me to move them back here so that D75511 is just about removing the function? :-) Should I have three patches: add the operator, change all users over, remove the function?

[ekatz]

llvm/lib/IR/ConstantFold.cpp

989 ↗

(On Diff #246916)

I don't think there will be a need for a third patch, as there are only few places where you change neg to operator-.
I think moving it to D75511 is simple/good enough.

[foad]

Don't update any existing users of neg.

[foad]

llvm/lib/IR/ConstantFold.cpp

989 ↗

(On Diff #246916)

Done, though I still don't understand why it's preferable.

[ekatz]

LGTM.

llvm/lib/IR/ConstantFold.cpp

989 ↗

(On Diff #246916)

I think this way we get a simpler and stricter change, which all purpose is to add the new functionality (without changing any of the old one yet). And another change for changing/removing the old functionality.
Hope it makes sense.

llvm/include/llvm/ADT/APFloat.h

public:
   }
 
   // TODO: bool parameters are not readable and a source of bugs.
   // Do something.
   opStatus next(bool nextDown) {
     APFLOAT_DISPATCH_ON_SEMANTICS(next(nextDown));
   }
 
+  /// Negate an APFloat.
+  APFloat operator-() const {
+    APFloat Result(*this);
+    Result.changeSign();
+    return Result;
+  }
+
   /// Add two APFloats, rounding ties to the nearest even.
   /// No error checking.
   APFloat operator+(const APFloat &RHS) const {
     APFloat Result(*this);
     (void)Result.add(RHS, rmNearestTiesToEven);
     return Result;
   }
 

 
 /// 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;
   return (B.compare(A) == APFloat::cmpLessThan) ? B : A;
 }

llvm/unittests/ADT/APFloatTest.cpp

TEST(APFloatTest, neg) {
   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));
+  EXPECT_TRUE(NegQNaN.bitwiseIsEqual(-QNaN));
+  EXPECT_TRUE(QNaN.bitwiseIsEqual(-NegQNaN));
 }
 
 TEST(APFloatTest, ilogb) {
   EXPECT_EQ(-1074, ilogb(APFloat::getSmallest(APFloat::IEEEdouble(), false)));
   EXPECT_EQ(-1074, ilogb(APFloat::getSmallest(APFloat::IEEEdouble(), true)));
   EXPECT_EQ(-1023, ilogb(APFloat(APFloat::IEEEdouble(), "0x1.ffffffffffffep-1024")));
   EXPECT_EQ(-1023, ilogb(APFloat(APFloat::IEEEdouble(), "0x1.ffffffffffffep-1023")));
   EXPECT_EQ(-1023, ilogb(APFloat(APFloat::IEEEdouble(), "-0x1.ffffffffffffep-1023")));