[PATCH] D95982: [CSSPGO] Unblock optimizations with pseudo probe instrumentation.

[Hongtao Yu via Phabricator via llvm-commits]

hoy marked an inline comment as done.
hoy added inline comments.


================
Comment at: llvm/lib/CodeGen/TwoAddressInstructionPass.cpp:805-809
     if (OtherMI.isDebugInstr())
       continue;
+    // Pseudo probe instructions cannot be counted against the limit.
+    if (OtherMI.isPseudoProbe())
+      continue;
----------------
wmi wrote:
> Similar to IR pass, add a MachineInstr::isDebugOrPseudoInstr()?
Good suggestion, thanks.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:152-157
+      // A pseudo probe call shouldn't change any function attribute since it
+      // doesn't translate to a real instruction. It comes with a memory access
+      // tag to prevent itself being removed by optimizations and not block
+      // other instructions being optimized.
+      if (isa<PseudoProbeInst>(I))
+        continue;
----------------
wmi wrote:
> Make it a little more general?
> 
>   if (auto *CB = dyn_cast<CallBase>(I))
>     if (CB->onlyAccessesInaccessibleMemory())
>       continue;
I think we might want to be a bit conservative here, since in theory `onlyAccessesInaccessibleMemory` still accesses memory. This may lead to
1. Functions with `onlyAccessesInaccessibleMemory`  calls may not be free to be removed.
2. Calls with `onlyAccessesInaccessibleMemory` may not be moved around across each other.

Pseudo probe don't have such effects thus checking against pseudo probes here should have minimal impact. What do you think?




================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:595
   for (++BBI; BBI != E; ++BBI)
-    if (BBI->mayWriteToMemory())
+    if (BBI->mayWriteToMemory() && !isa<PseudoProbeInst>(BBI))
       return false;
----------------
wmi wrote:
> Make it a little more general in the same way as above.
Sounds good to generalize here since the `LoadInst` to be sinked doesn't access inaccessible memory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95982