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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1986
+    if (CurI.mayHaveSideEffects() && !isa<StoreInst>(CurI) &&
+        !isa<PseudoProbeInst>(CurI))
       return nullptr;
----------------
hoy wrote:
> wmi wrote:
> > hoy wrote:
> > > hoy wrote:
> > > > wmi wrote:
> > > > > Can we check CallBase::onlyAccessesInaccessibleMemory instead of checking PseudoProbeInst here?
> > > > Good question. Checking against `PseudoProbeInst` is more than `onlyAccessesInaccessibleMemory`. The instruction identified here will be either converted to a `select` or deleted. I probably should move the `PseudoProbeInst` check into `BrBB->instructionsWithoutDebug()`.
> > > On the second thought, it might not be a good idea to fuse the check into `instructionsWithoutDebug` since API is used in quite some places. We sometimes like a `PseudoProbeInst` treated like a debug intrinsic but sometimes not. It is specific to the transform we'd like to not block.
> > I think it is a good idea to fuse the check into instructionsWithoutDebug. That will make psuedoProbe intrinsic behave more like debug intrinsic and block less transformations, like we discussed in D86193. What is the case you have concern about?
> > 
> > 
> I was thinking that some passes may just use `instructionsWithoutDebug` to ignore and remove debug instructions. We may want that happening to pseudo probe selectively. By searching where the API is used, it looks like it's fine have them handle pseudo probes as well. I'm testing to see if there's a regression to the profile quality.
Test result looked OK. Moved the check into `instructionsWithoutDebug` .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86490