[PATCH] D140181: DAG: Pull fneg out of select feeding fadd into fsub

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7013
+    SDValue RHS = Op.getOperand(2);
+    if (LHS.getOpcode() == ISD::FNEG && RHS.getOpcode() == ISD::FNEG) {
+      return DAG.getNode(ISD::SELECT, DL, VT, Op.getOperand(0),
----------------
RKSimon wrote:
> RKSimon wrote:
> > arsenm wrote:
> > > arsenm wrote:
> > > > arsenm wrote:
> > > > > RKSimon wrote:
> > > > > > Why don't you want to use getNegatedExpression ?
> > > > > This is in getNegatedExpression and it didn’t occur to be to use it recursively 
> > > > Of course the first case I try with this infinite loops
> > > I can't figure out how to represent this with the costs here. If I have this:
> > > 
> > > 
> > > ```
> > > define float @fadd_select_fsub_fsub_f32(i32 %arg0, float %x, float %y, float %z) {
> > >   %cmp = icmp eq i32 %arg0, 0
> > >   %neg.x = fsub nsz float %x, 2.0
> > >   %neg.y  = fsub nsz float %y, 2.0
> > >   %select = select i1 %cmp, float %neg.x, float %neg.y
> > >   %add = fadd float %select, %z
> > >   ret float %add
> > > }
> > > 
> > > ```
> > > 
> > > I'd expect to swap the fsubs and turn the fadd into an fsub. If I report Cheaper here, that happens. But then, the new fsub is turned right back into the fadd. The cost is only obviously cheaper if the cost of the negate in the select operands is canceled out by the negate of the fadd user. I'm not sure whether I should try to be hacking either the fadd or fsub combines, or making the cost system more sophisticated.
> > > 
> > > I also don't understand why  (fneg (fsub X, Y)) -> (fsub Y, X) is only considered neutral, and not cheaper if fsub is legal
> > > 
> > ```
> >     // fold (fneg (fsub X, Y)) -> (fsub Y, X)
> >     Cost = NegatibleCost::Neutral;
> >     return DAG.getNode(ISD::FSUB, DL, VT, Y, X, Flags);
> > ```
> > The use of the fneg there is confusing - AFAICT the negation of a fsub should be neutral, as it just commutes the operands, its more likely that the calling op hasn't set its costs correctly.
> > 
> > Please can you update the patch with your WIP code - for instance "Cost = NegatibleCost::Cheaper;" isn't correct for the select, it needs to determine its cost from the costs returned by the getNegatedExpression calls.
> This is what I came up with:
> ```
>   case ISD::SELECT:
>   case ISD::VSELECT: {
>     // fold (fneg (select C, LHS, RHS)) -> (select C, (fneg LHS), (fneg RHS))
>     // iff at least one cost is cheaper and the other is neutral/cheaper
>     SDValue LHS = Op.getOperand(1);
>     NegatibleCost CostLHS = NegatibleCost::Expensive;
>     SDValue NegLHS =
>         getNegatedExpression(LHS, DAG, LegalOps, OptForSize, CostLHS, Depth);
>     if (!NegLHS || CostLHS > NegatibleCost::Neutral) {
>       RemoveDeadNode(NegLHS);
>       break;
>     }
> 
>     // Prevent this node from being deleted by the next call.
>     Handles.emplace_back(NegLHS);
> 
>     SDValue RHS = Op.getOperand(2);
>     NegatibleCost CostRHS = NegatibleCost::Expensive;
>     SDValue NegRHS =
>         getNegatedExpression(RHS, DAG, LegalOps, OptForSize, CostRHS, Depth);
> 
>     // We're done with the handles.
>     Handles.clear();
> 
>     if (!NegRHS || CostRHS > NegatibleCost::Neutral ||
>         (CostLHS != NegatibleCost::Cheaper &&
>          CostRHS != NegatibleCost::Cheaper)) {
>       RemoveDeadNode(NegLHS);
>       RemoveDeadNode(NegRHS);
>       break;
>     }
> 
>     Cost = std::min(CostLHS, CostRHS);
>     return DAG.getSelect(DL, VT, Op.getOperand(0), NegLHS, NegRHS);
>   }
> ```
Doesn't handle the cases I wrote to see if the recursive calls did anything which were only cheaper if you consider the elimination of the negate of the final select. This is good enough for me though, it catches a bit more than what I had.

Do you want me to push the baseline tests and you push this?


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

https://reviews.llvm.org/D140181