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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:250
+    // instructions in the function and penalty for specializing more functions.
+    unsigned Penalty = (NumFuncSpecialized + 1);
+    return Metrics.NumInsts * InlineConstants::InstrCost * Penalty;
----------------
`NumFuncSpecialized` is defined as `STATISTIC(NumFuncSpecialized, "Number of Functions Specialized");` How will this work when LLVM is build without statistics? (also redundant brackets)


================
Comment at: llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp:65
+// transition to ValueLatticeElement.
+static bool isConstant(const ValueLatticeElement &LV) {
+  return LV.isConstant() ||
----------------
fhahn wrote:
> 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)
Looks like this was not addressed. If the function is kept, please at least update the comment, as it is mis-leading at the moment (it also returns false if `LV` is a constant range with more than a single element, see the comment for the same function in `SCCP.cpp`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93838