[PATCH] D150464: [FuncSpec] Improve the accuracy of the cost model.

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:100-103
+static Cost estimateBasicBlocks(SmallVectorImpl<BasicBlock *> &WorkList,
+                                ConstMap &KnownConstants, SCCPSolver &Solver,
+                                BlockFrequencyInfo &BFI,
+                                TargetTransformInfo &TTI) {
----------------
ChuanqiXu wrote:
> Let's add a comment for this since its semantics are not clear from the signature.
Will do.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:210
+  // it is executable and has a unique predecessor.
+  SmallVector<BasicBlock *, 32> WorkList;
+  if (Solver.isBlockExecutable(Succ) &&
----------------
ChuanqiXu wrote:
> 
I don't understand. The worklist is populated by a chain of basic blocks. If 32 is too much perharps we could leave it to default? Surely 1 is too conservative and expanding the small vector at runtime seems expensive.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:234-236
+    if (!C) {
+      AllOperandsAreConst = false;
+      break;
----------------
ChuanqiXu wrote:
> Could we refactor it into this? Then we can remove `AllOperandsAreConst`.
Oh yes, I had the old impementation (pre InstructionVisitor) in mind at the time of writing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150464