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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Yeah, nice patch.

Just a few nits inlined when I was reading the patch.

According to my definition of NFC, this is not so NFC as things are done quite differently now. I appreciate the codegen should be the same, which is the definition of NFC that some use I believe.

Just out of curiousity, did you measure compile times improvements with this, which I guess is the reason for doing this?



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:429
+    SmallVector<Argument *, 4> Args;
+    for (Argument &Arg : F->args()) {
+      if (isArgumentInteresting(&Arg))
----------------
Nit: you can also drop these brackets I think


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:434
+
+    if (Args.size() == 0)
+      return false;
----------------
Nit: not sure the coding standard says anything about comparing against zero, but personally it's a bit verbose for me and I prefer `!Args.size()`.


================
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;
----------------
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? :)


================
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
----------------
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.


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

https://reviews.llvm.org/D135968