Page MenuHomePhabricator

[APFloat] Overload unary operator-
ClosedPublic

Authored by foad on Feb 27 2020, 3:53 AM.

Details

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).

Diff Detail

Event Timeline

foad created this revision.Feb 27 2020, 3:53 AM
RKSimon added inline comments.Feb 27 2020, 4:22 AM
llvm/unittests/ADT/APFloatTest.cpp
3001

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

foad updated this revision to Diff 246916.Feb 27 2020, 5:01 AM

Test neg(A) as well as -A.

foad marked 2 inline comments as done.Feb 27 2020, 5:02 AM
foad added inline comments.
llvm/unittests/ADT/APFloatTest.cpp
3001

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

jfb added a reviewer: scanon.Feb 27 2020, 9:36 AM
ekatz added inline comments.Mar 2 2020, 8:08 AM
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 marked 3 inline comments as done.Mar 2 2020, 8:27 AM
foad added inline comments.
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 added inline comments.Mar 2 2020, 11:11 AM
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 updated this revision to Diff 247844.Mar 3 2020, 3:22 AM

Move unary operator- next to other overloads.

foad marked 4 inline comments as done.Mar 3 2020, 3:26 AM
foad added inline comments.
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 added inline comments.Mar 3 2020, 6:18 AM
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 marked 3 inline comments as done.Mar 3 2020, 6:29 AM
foad added inline comments.
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 added inline comments.Mar 5 2020, 9:11 AM
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 updated this revision to Diff 248584.Mar 5 2020, 12:55 PM

Don't update any existing users of neg.

foad marked 3 inline comments as done.Mar 5 2020, 12:57 PM
foad added inline comments.
llvm/lib/IR/ConstantFold.cpp
989 ↗(On Diff #246916)

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

ekatz accepted this revision.Mar 5 2020, 8:49 PM

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.

This revision is now accepted and ready to land.Mar 5 2020, 8:49 PM
This revision was automatically updated to reflect the committed changes.
foad marked an inline comment as done.