[PATCH] D78291: [NFC][DAGCombine] Adding three helper functions and change the getNegatedExpression to negateExpression

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang marked an inline comment as done.
steven.zhang added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12693
+  SDValue NegN0 =
+      TLI.getCheaperNegatedExpression(N0, DAG, LegalOperations, ForCodeSize);
+  SDValue NegN1 =
----------------
steven.zhang wrote:
> steven.zhang wrote:
> > This lines of code is a bit tricky, we can write it as
> > ```
> >   TargetLowering::NegatibleCost CostN0 =
> >       TargetLowering::NegatibleCost::Expensive;
> >   TargetLowering::NegatibleCost CostN1 =
> >       TargetLowering::NegatibleCost::Expensive;
> >   SDValue NegN0 =
> >       TLI.getNegatedExpression(N0, DAG, LegalOperations, ForCodeSize, CostN0);
> >   SDValue NegN1 =
> >       TLI.getNegatedExpression(N1, DAG, LegalOperations, ForCodeSize, CostN1);
> >   if (NegN0 && NegN1 &&
> >       (CostN0 == TargetLowering::NegatibleCost::Cheaper ||
> >        CostN1 == TargetLowering::NegatibleCost::Cheaper)) {
> >     return DAG.getNode(ISD::FMUL, DL, VT, NegN0, NegN1, Flags);
> >   }
> > ```
> > However, they are not semantics the same, as current implementation don't want to negate the expression if the cost of negate N0 and N1 are both neutral while the above implementation did. Technical speaking, it won't have bad impact as we don't change the FMUL if both are neutral. However, it changes the order of the node to combine, which change the PowerPC test llvm/test/CodeGen/PowerPC/qpx-recipest.ll we see in D77319  Maybe, we need another patch to change the semantics and PowerPC test.
> I suddenly realize that, the change here is still not strict-NFC, because we change the DAG when trying to get the cheaper negated expression of N0, which will affect the N1. I will leave this part unchanged and do it in D77319.
Do it in D78347 I mean.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78291