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

[Simon Pilgrim via Phabricator via llvm-commits]

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


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

https://reviews.llvm.org/D140181