[PATCH] D75501: [DAGCombine] Check the uses of negated floating constant and remove the hack

[qshanz via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5570-5575
+  if (!Op.hasOneUse() &&
+      !(Op.getOpcode() == ISD::FP_EXTEND &&
+        isFPExtFree(VT, Op.getOperand(0).getValueType())) &&
+      !(Op.getOpcode() == ISD::ConstantFP &&
+        !getNegatedExpression(Op, DAG, LegalOperations, ForCodeSize)
+             .hasOneUse()))
----------------
RKSimon wrote:
> spatel wrote:
> > 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;
> > 
> > 
> > ```
> ```
> 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;
> }
> ```
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".



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75501