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

[Joseph Faulls via Phabricator via llvm-commits]

Joe added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:2577
+    // already be constant.
+    if (!Solver.getLatticeValueFor(A).isOverdefined())
+      return false;
----------------
What if the LatticeValue is a ConstantRange? There could be some great specialization opportunities there. Currently, only checking for overdefined leads to:

```

// specialized
foo(a)
foo(1)
foo(2)

// not specialized
foo(1)
foo(2)
foo(3)
```


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:2586
+    SmallVector<Constant *, 4> PossibleConstants;
+    bool AllConstant = getPossibleConstants(A, PossibleConstants);
+    if (PossibleConstants.empty() ||
----------------
Should getPossibleConstants only find //unique// possible constants? Currently it won't specialize If you have over the threshold of calls using the same argument (e.g foo(a) x 4 and foo(b) x 1)


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:2588
+    if (PossibleConstants.empty() ||
+        PossibleConstants.size() > MaxConstantsThreshold)
+      return false;
----------------
The major problem I see with a simple cutoff threshold like this would be the following:

10x foo(a)
1x foo(b)
1x foo(c)
1x foo(d)

There could be huge benefit for specializing for foo(a) here. A potential change here I see is for PossibleConstants to capture the number of occurrences (possibly taking into account loops), and to specialize the function up to MaxConstantsThreshold in descending count order.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:2644-2647
+      if (V != A && !Solver.getLatticeValueFor(V).isConstant()) {
+        AllConstant = false;
+        continue;
+      }
----------------
If wanting to support ConstantRanges as per the other comment, this would need changing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93838