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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
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)) {
----------------
fhahn wrote:
> 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?
I might not get your suggestion, but the solving is done much earlier. I.e., `RunSCCPSolver` is done in `runFunctionSpecialization`. This here is the final step that performs the actual transformation.


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:315-317
+    uint64_t Cost =
+        TTI.getUserCost(U, TargetTransformInfo::TCK_SizeAndLatency) ==
+        TargetTransformInfo::TCC_Free;
----------------
ChuanqiXu wrote:
> Should this be `!=` instead? Or I think we should use `TTI.getUserCost` for the initial value directly. Now if `getUserCost` returns non-zero, the value for Cost would become 0! It is too weird.
Yeah, am fixing and rewriting this to use `InstructionCost`.


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:637
+
+// This transformation applied on this example:
+//
----------------
fhahn wrote:
> Why do we need this transformation here? Is this required for specialization or to catch the MCF case?
It's also not yet covered by tests, so I will remove it for now. Even if it is needed for MCF (can't remember), then it looks like a nice candidate as a follow up once we've got something in.


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:798
+    // Replace some unresolved constant arguments
+    tryToReplaceConstantArgOperand(FuncDecls, M, Solver);
+
----------------
fhahn wrote:
> Is it possible to only replace values once we are completely done with solving? 
Will remove this (see also earlier comment); this is a bit of a different optimisation that we can look at later.


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

https://reviews.llvm.org/D93838