This is an archive of the discontinued LLVM Phabricator instance.

DAG: Pull fneg out of select feeding fadd into fsub
ClosedPublic

Authored by arsenm on Dec 15 2022, 5:20 PM.

Details

Summary

Enables folding fadd x, (select c, (fneg a), (fneg b))

-> fsub (select a, b), c

Avoids some regressions in a future AMDGPU change.

Diff Detail

Event Timeline

arsenm created this revision.Dec 15 2022, 5:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2022, 5:20 PM
arsenm requested review of this revision.Dec 15 2022, 5:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2022, 5:20 PM
Herald added a subscriber: wdng. · View Herald Transcript
RKSimon added inline comments.Dec 16 2022, 2:32 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7012

Why don't you want to use getNegatedExpression ?

arsenm added inline comments.Dec 16 2022, 4:08 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7012

This is in getNegatedExpression and it didn’t occur to be to use it recursively

arsenm added inline comments.Dec 16 2022, 5:29 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7012

Of course the first case I try with this infinite loops

arsenm added inline comments.Dec 16 2022, 6:32 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7012

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

RKSimon added inline comments.Dec 16 2022, 6:46 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7012
// 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.

RKSimon added inline comments.Dec 17 2022, 4:29 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7012

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);
}
arsenm added inline comments.Dec 18 2022, 3:10 PM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7012

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?

arsenm updated this revision to Diff 483846.Dec 18 2022, 3:10 PM

Add some more tests and copy suggested patch

RKSimon accepted this revision.Dec 19 2022, 3:06 AM

LGTM - happy for you to push the baseline tests and then please push my change as well

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7012

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.

Please can you raise a bug for that test case and I'll try to take a look at it.

This revision is now accepted and ready to land.Dec 19 2022, 3:06 AM