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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 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.
----------------
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.


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

https://reviews.llvm.org/D107711