[PATCH] D107711: [DAGCombiner] Add target hook function to decide folding (mul (add x, c1), c2)

Ben Shi via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 11 19:13:26 PDT 2021


benshi001 added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2086
+  /// The target should check the cost of materializing c1, c2 and c1*c2 into
+  /// registers. If it is not sure about some cases, a default true
+  /// can be returned to let the DAGCombiner decide.
----------------
jrtc27 wrote:
> benshi001 wrote:
> > benshi001 wrote:
> > > lebedev.ri wrote:
> > > > 
> > > I think my origin `default true` is right, the default return value should not be false.
> > > 
> > > Since my hook is called as
> > > ```
> > >   if (AddNode.getNode()->hasOneUse() &&
> > >       TLI.isMulAddWithConstProfitable(AddNode, ConstNode))
> > >     return true;
> > > ```
> > > 
> > > So it should return default true for undetermined cases.
> > > 
> > > 
> > Actually my previous version is `isMulAddWithConstNotProfitable`, which return default false, and return true for clear regression on specific targets.
> > 
> > Craig suggested me to remove the `Not`, and inverse the condition when calling.
> > 
> > The core issue is, the original DAGCombiner will do the folding if the AddNode has only one use, which will harm performance in some situation. And the solution is adding another check (along with the hasOneUse) to let the target prevent the transform if the target does think there is regresssion.
> > 
> > But if the target is also not sure, what default value should be ? And the hook name should have `Not` or should not have a `Not` ? 
> The default should be whatever makes SelectionDAG behave the same as it currently does unless the changes in behaviour turn out to be useful for the majority of targets.
> 
> TLI functions should be positive not negative; there are no hooks that have Not in them (other than when they refer to a not instruction).
> 
> I'm not sure what the issue is though? Whether you have a NotProfitable function that defaults to false or a Profitable function that defaults to true makes no semantic difference other than when the caller needs to put a ! in front of it, it's purely a stylistic issue.
I will keep current form without `Not`, and still return default true to "make SelectionDAG behave the same as it currently does".

And I have improved my comments more clear as

```
  /// Return true if it may be profitable to fold
  /// (mul (add x, c1), c2) -> (add (mul x, c2), c1*c2), and return false
  /// to prevent the folding for definite regression.
  /// The target should check the cost of materializing c1, c2 and c1*c2 into
  /// registers. If it is not sure about some cases, a default true
  /// can be returned to let the DAGCombiner decide.
```


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

https://reviews.llvm.org/D107711



More information about the llvm-commits mailing list