[PATCH] D109294: [Inline] Introduce Constant::hasOneLiveUse, use it instead of hasOneUse in inline cost model (PR51667)

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

thanks for doing this, some small comments, otherwise lgtm



================
Comment at: llvm/include/llvm/IR/Constant.h:211
+  /// Constant::removeDeadConstantUsers, but doesn't remove dead constants.
+  bool hasNLiveUses(unsigned N) const;
+
----------------
is this currently needed anywhere other than the implementation of hasOneLiveUse? I'd recommend removing it - we can always introduce it later.



================
Comment at: llvm/include/llvm/IR/Constant.h:217
+  /// Constant::removeDeadConstantUsers, but doesn't remove dead constants.
+  bool hasNLiveUsesOrMore(unsigned N) const;
+
----------------
is this used anywhere? If not, no need to introduce it. it'd also keep the implementation of constantHasLiveUses simpler.


================
Comment at: llvm/lib/IR/Constants.cpp:720
+///
+/// This should be kept in sync with constantIsDead.
 static bool removeDeadUsersOfConstant(const Constant *C) {
----------------
If you're concerned about the fact that both have the same traversal logic, rather than a comment (which is fragile to change), you could, for example, have a utility common to both removeDeadUsersOfConstant and to constantIsDead that takes a function_ref for what to do after the while/for loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109294