Page MenuHomePhabricator

[DAGCombine] Check the uses of negated floating constant and remove the hack
ClosedPublic

Authored by steven.zhang on Mon, Mar 2, 11:19 PM.

Details

Summary

This patch is motivated by the case I added in PowerPC/fma-combine.ll, and we hit an assertion here due to somewhat the same reason as https://reviews.llvm.org/D70975 . The case is narrowed down from spec and it is important to us. Though we have some hack, it still failed with my case, as the operand 0 now is NOT a const fp, it is another fma that with const fp. And that const fp is negated which result in multi-uses.

So, I think a better fix is to check the uses of the negated const fp. If there are already use of its negated value, we will have benefit as no extra Node is added.

Diff Detail

Event Timeline

steven.zhang created this revision.Mon, Mar 2, 11:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Mar 2, 11:19 PM
spatel added inline comments.Tue, Mar 3, 5:59 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5554–5568

This is difficult to read. Please add some intermediate names (maybe as shown below).

It's not clear to me if that's the logic we want - is this a good or better fix for the problem?

bool IsFreeExtend = Op.getOpcode() == ISD::FP_EXTEND &&
                    isFPExtFree(VT, Op.getOperand(0).getValueType());
bool IsFreeConstant =
    Op.getOpcode() == ISD::ConstantFP &&
    !getNegatedExpression(Op, DAG, LegalOperations, ForCodeSize).use_empty();
if (!Op.hasOneUse() && !IsFreeExtend && !IsFreeConstant)
  return NegatibleCost::Expensive;
RKSimon added inline comments.Tue, Mar 3, 8:53 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5554–5568
if (!Op.hasOneUse()) {
  bool IsFreeExtend = Op.getOpcode() == ISD::FP_EXTEND &&
                    isFPExtFree(VT, Op.getOperand(0).getValueType());
  if (!IsFreeExtend)
    return NegatibleCost::Expensive;

  bool IsFreeConstant = Op.getOpcode() == ISD::ConstantFP &&
        !getNegatedExpression(Op, DAG, LegalOperations, ForCodeSize).use_empty();
  if (!IsFreeConstant)
    return NegatibleCost::Expensive;
}

Update the patch to make it more clear and fix an issue of counting the uses.

steven.zhang marked an inline comment as done.Tue, Mar 3, 6:24 PM
steven.zhang added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5554–5568

Yes, that is exactly what I mean, thank you for the code and I have updated the patch.
Regarding to spatel's question, currently, we are doing special handling for const fp when we don't know which one has better cost. My idea is to remove this special handling and improve the evaluation of the cost for const fp to make it more precise, then reduce the chance that we cannot determine which one is better. Does it make sense ?

And moreover, the special handling cannot handle all the cases. I have added another case in my patch to demonstrate the issue. This is what my case looks like:

t0  =   t1   x    t2   +    t3
        +                   +
        v                   v
 t3  x 42.0 - 1.0       t4  +  1.0
  +
  v
 -t4

We are now negating the expression "t0", so t3 is negated first, which is "-t4 - 1.0". And then, we need to decide to negate t1 or t3 which hit the hack code. So, we call the getNegatibleCost to make the decision. Unfortunately, both are expensive as the operand "-1.0" of expression "t1" now has multiple use. The hack here won't help as it is just checking "t1" and "t2".

Fix the logic bug ...

spatel accepted this revision.Wed, Mar 4, 4:59 AM

LGTM

This revision is now accepted and ready to land.Wed, Mar 4, 4:59 AM
This revision was automatically updated to reflect the committed changes.