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

[Ben Shi via Phabricator via llvm-commits]

benshi001 marked 2 inline comments as done.
benshi001 added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16857
 
-  // If the add only has one use, this would be OK to do.
-  if (AddNode.getNode()->hasOneUse())
+  // If the add only has one use and the target finds no regression, this
+  // would be OK to do.
----------------
jrtc27 wrote:
> "finds no regression" doesn't make sense to me. A regression is a bug, but if the target says it's profitable then there is no regression; a regression would be if a target said it was not profitable but it in fact was, and so the emitted code got worse. So when a target returns false, it's not that it finds a regression, because nothing has happened yet.
> 
> This also feeds into a related point. When comments in DAGCombiner talk about regressions, they generally mean "this transformation should make sense on most targets, but many of them don't enable it currently because they have patterns or custom lowering that would need to be adapted to handle it otherwise they won't match important cases any more" (with the "won't match important cases" being the regression), generally marked as TODO (all but one, with the odd-one-out still saying that something should probably be improved). This is not that case, this is just asking the target whether the transformation is profitable, simply as a "does your instruction set benefit from this transformation?".
Thanks. I have made the comments more clear.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.h:464
 
+  /// 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:
> What's the point of duplicating this doxygen comment? It's just going to get outdated, and anyone who wants to know what it does (which is pretty obvious from the name) can just look at TargetLowering.h. If we copied the documentation to every override the tree would be a mess.
Thanks. I have removed the redundant comments.


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

https://reviews.llvm.org/D107711