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

[Ben Shi via Phabricator via llvm-commits]

benshi001 marked an inline comment as done.
benshi001 added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2083
 
+  /// Return true if it may be profitable to fold
+  /// (mul (add x, c1), c2) -> (add (mul x, c2), c1*c2), and return false
----------------
jrtc27 wrote:
> The comment's still not great, putting grammatical issues aside. A lot of it is just explaining the basics of TLI hooks, but also is overly prescriptive with what backends should do to evaluate it (and I also don't like "to avoid definite worse code generated", often TLI hooks end up being best-effort heuristics, unable to give a definitive answer, because that might require extremely expensive whole-function checks that depend on knowing what other transformations are going to be made).
> 
> I'd just go with something like (borrowing style from surrounding examples):
> 
> > Return true if it may be profitable to transform
> > (mul (add x, c1), c2) -> (add (mul x, c2), c1*c2).
> > This may not be true if c1 and c2 can be represented as immediates but c1*c2 cannot, for example.
Thanks, I have updated the comments according to your suggested expression.

One more issue, English is not my mother language and your are appreciated to help me fix any "grammatical issues" you mentioned. ^_^


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

https://reviews.llvm.org/D107711