[PATCH] D135968: [FuncSpec][NFC] Refactor finding specialisation opportunities

[Momchil Velikov via Phabricator via llvm-commits]

chill added a comment.

Unfortunately, this patch exposes a latent issue in the pass - one call site matching two or more specialisations - as can be seen in the failure of `llvm/test/Transforms/FunctionSpecialization/identical-specializations.ll`.



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:670-672
-    // No point in specialization if the argument is unused.
-    if (A->user_empty())
-      return false;
----------------
SjoerdMeijer wrote:
> chill wrote:
> > ChuanqiXu wrote:
> > > Why this check missed?
> > Oops, that's accidental. I'll put it back.
> Looks like we are missing a test case? :)
Ack!


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:676
+  /// of the formal parameter \p A.
+  bool isArgumentInteresting(Argument *A) {
     // For now, don't attempt to specialize functions based on the values of
----------------
SjoerdMeijer wrote:
> After reducing/changing the comments, the function name and comment are a bit out of sync now for me, because specialisation should not only "possible" (the new comment), but also profitable which is why "interesting" is in the function name. Perhaps restoring some words about the profitability explains the "interesting" part.
In fact, the old comment was out of sync  - neither the old `isArgumentInteresting` nor the old `getPossibleConstants` did any cost/benefit analysis.


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

https://reviews.llvm.org/D135968