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

[Ben Shi via Phabricator via llvm-commits]

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.
----------------
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` ? 


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

https://reviews.llvm.org/D107711