[PATCH] D93838: [SCCP] Add Function Specialization pass

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D93838#2770480 <https://reviews.llvm.org/D93838#2770480>, @nikic wrote:

> I'm pretty sure @fhahn did not mean to suggest that the pass is actually default enabled when it lands, merely that there is some viable path towards enabling it in the future. Adding a pass and enabling it in the default pipeline are always two separate steps.

This was indeed how my earlier comments were intended. Some comments on the code itself.



================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:1
+//===- SCCP.cpp - Sparse Conditional Constant Propagation -----------------===//
+//
----------------
this needs updating.


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:15
+// - It does not handle specialization of recursive functions,
+// - It does not yet handle constants and constant ranges,
+// - Only 1 argument per function is specialised,
----------------
this sounds a bit odd. The first sentence says it handles constant parameters. I guess you mean non-constant-int constants?


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:46
+static cl::opt<unsigned> FuncSpecializationMaxIters(
+    "func-specialization-max-iters", cl::Hidden,
+    cl::desc("The maximum number of iterations function specialization is run"),
----------------
need test for the option?


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:57
+static cl::opt<unsigned>
+    AvgLoopIterationCount("func-specialization-avg-iters-cost", cl::Hidden,
+                          cl::desc("Average loop iteration count cost"),
----------------
needs test for the option?


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:65
+// transition to ValueLatticeElement.
+static bool isConstant(const ValueLatticeElement &LV) {
+  return LV.isConstant() ||
----------------
Those were added to transition existing code in SCCP to `ValueLatticeELement`. Ideally new code would be explicit about what they expect (constant-range vs constant-int)


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:130
+      // Replace the function arguments for the specialized functions.
+      for (Argument &Arg : SpecializedFunc->args()) {
+        if (!Arg.use_empty() && tryToReplaceWithConstant(&Arg)) {
----------------
Instead of patching up the IR, can we just set the lattice values for the cloned function arguments accordingly until we are done with solving?


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:172
+  /// Clone the function \p F and remove the ssa_copy intrinsics added by
+  /// the SCCPSolver in the cloned version.
+  Function *cloneCandidateFunction(Function *F) {
----------------
Could you explain why we need to remove `ssa_copy` in the clone function?


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:212
+    if (F->hasOptSize() ||
+        llvm::shouldOptimizeForSize(F, nullptr, nullptr, PGSOQueryType::IRPass))
+      return false;
----------------
nit: no `llvm::` should be needed.


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:637
+
+// This transformation applied on this example:
+//
----------------
Why do we need this transformation here? Is this required for specialization or to catch the MCF case?


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:766
+
+    for (auto *F : WorkList) {
+      for (BasicBlock &BB : *F) {
----------------
Why do we need to modify the IR after each call to `RunSCCPSolver` rather than after all solving is done? 


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:777
+  auto &TrackedFuncs = Solver.getArgumentTrackedFunctions();
+  SmallVector<Function *, 16> FuncDecls(TrackedFuncs.begin(),
+                                        TrackedFuncs.end());
----------------
Are those declarations? Shouldn't we only track functions with definitions?


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:798
+    // Replace some unresolved constant arguments
+    tryToReplaceConstantArgOperand(FuncDecls, M, Solver);
+
----------------
Is it possible to only replace values once we are completely done with solving? 


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:30
 #include "llvm/ADT/Statistic.h"
+#include "llvm/Analysis/AssumptionCache.h"
+#include "llvm/Analysis/CodeMetrics.h"
----------------
All those includes are not needed in the file?


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:170
+      if (Inst.isSafeToRemove()) {
+        Solver.removeLatticeValueFor(&Inst);
         Inst.eraseFromParent();
----------------
Why is this needed?


================
Comment at: llvm/test/Transforms/FunctionSpecialization/function-specialization4.ll:7
+
+target triple = "aarch64-unknown-linux-gnueabi"
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
----------------
do those tests depend on information from the AArch64 backend? If so, they should only be executed if the AArch64 backend is built.


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

https://reviews.llvm.org/D93838