[PATCH] D75236: [APFloat] Overload unary operator-

[Ehud Katz via Phabricator via llvm-commits]

ekatz accepted this revision.
ekatz added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/lib/IR/ConstantFold.cpp:989
     case Instruction::FNeg:
-      return ConstantFP::get(C->getContext(), neg(CV));
+      return ConstantFP::get(C->getContext(), -CV);
     }
----------------
foad wrote:
> ekatz wrote:
> > foad wrote:
> > > RKSimon wrote:
> > > > foad wrote:
> > > > > ekatz wrote:
> > > > > > foad wrote:
> > > > > > > ekatz wrote:
> > > > > > > > Why change it? Calling `operator-()` seems a bit more elusive, than the `neg`.
> > > > > > > > (This also regards to the changes in "TargetLowering.cpp".)
> > > > > > > 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-`!
> > > > > > 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.
> > > > > 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.
> > > > 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.
> > > 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?
> > 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.
> Done, though I still don't understand why it's preferable.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75236/new/

https://reviews.llvm.org/D75236