[PATCH] D86490: [CSSPGO] IR instrinsic for pseudo-probe block instrumentation

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D86490#2360454 <https://reviews.llvm.org/D86490#2360454>, @davidxl wrote:

> How about inline cost analysis? It needs to skip the new instructions. Similarly for the Partial inliner, the static cost of this should be set to zero.

Good point. Yes, pseudo probes should be excluded from inline cost analysis. We were planning to include the change in upcoming patches. Now I'm moving it here.



================
Comment at: llvm/include/llvm/IR/BasicBlock.h:185
+  /// PHINode, a debug intrinsic, or a pseudo probe intrinsic.
+  const Instruction *getFirstNonPHIOrDbgOrPseudoProbe() const;
+  Instruction *getFirstNonPHIOrDbgOrPseudoProbe() {
----------------
davidxl wrote:
> Is it possible to also need to skip both PseudoProbe and Lifetime Markers?
Good point. I just checked some uses of `getFirstNonPHIOrDbgOrLifetime` where pseudo probe should also be needed. I'm making a helper for that. I'm thinking about deferring the actual replacement work to when pseudoprobe is used in combination with those cases so that we have good testing there. What do you think?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:2239
     BasicBlock::iterator BI = BB->begin();
-    while (isa<DbgInfoIntrinsic>(BI)) ++BI;
+    while (isa<DbgInfoIntrinsic>(BI) || isa<PseudoProbeInst>(BI))
+      ++BI;
----------------
davidxl wrote:
> Perhaps introduce a helper function to skip non-code instructions
Changed to using `getFirstNonPHIOrDbgOrPseudoProbe`.


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:240
       CallInst *CI = dyn_cast<CallInst>(&I);
-      if (!CI || CI->isTailCall() || isa<DbgInfoIntrinsic>(&I))
+      // A PseudoProbeInst does access memory and will be marked as a tail call
+      // if we don't bail out here.
----------------
davidxl wrote:
> why does it access memory?
Because it has the `IntrInaccessibleMemOnly` flag. I changed the comment to be less confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86490