[PATCH] D126455: [FuncSpec] Make the Function Specializer part of the IPSCCP pass.

[Momchil Velikov via Phabricator via llvm-commits]

chill added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/CMakeLists.txt:97
   InstCombine
+  IPO
   Support
----------------
Why add IPO here ?


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:161-172
+  // Record uses of V to avoid visiting irrelevant uses of const later.
+  SmallVector<Instruction *> UseInsts;
+  for (auto *U : V->users())
+    if (auto *I = dyn_cast<Instruction>(U))
+      if (Solver.isBlockExecutable(I->getParent()))
+        UseInsts.push_back(I);
+
----------------
labrinea wrote:
> Just found that we need to do the same inside `replaceSignedInst()` too. I will move this code a function.
Would it be possible to call `markUsersAsChanged` here ?


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:237
   Solver.removeLatticeValueFor(&Inst);
-  Inst.eraseFromParent();
+  Inst.removeFromParent();
+  ToDelete.insert(&Inst);
----------------
Is there a specific reason to remove the instruction from the block?
If not, I'd suggest doing deletion in a single place, as opposed to spreading parts of it all over.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:254
+        Solver.removeLatticeValueFor(&Inst);
+        Inst.removeFromParent();
+        ToDelete.insert(&Inst);
----------------
Likewise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126455