[PATCH] D97481: [CSSPGO] Unblocking optimizations by dangling pseudo probes.

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

>> A general question, do we care about where we should put dangling probe? Currently dangling probes are scattered in some predecessor/successor blocks they don't belong to. Looks like they can be anywhere. Wondering whether it is better to collect all of them to some place (entry block?). I am not sure about it either. Just want to know what you think.
>
> That's a very good question. We don't care about where a dangling probe is placed technically. But in reality placing dangling probes around the original place they are from helps IR readability. It can help reason about why they are dangled.

Ok, that is reasonable.



================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:674
   /// or end().
-  iterator getFirstNonDebugInstr();
-  const_iterator getFirstNonDebugInstr() const {
-    return const_cast<MachineBasicBlock *>(this)->getFirstNonDebugInstr();
+  iterator getFirstNonDebugInstr(bool SkipPseudoOp = false);
+  const_iterator getFirstNonDebugInstr(bool SkipPseudoOp = false) const {
----------------
hoy wrote:
> wmi wrote:
> > In which case SkipPseudoOp needs to be true and which case to be false?
> Good question. `SkipPseudoOp` should be true when we are sure that the probes should not be ignored. For example, we don't want to merge blocks that look the same except for their pseudo probes because the merge will make it hard to infer the counts for the original edges. On the contrary, `SkipPseudoOp` should be false when we are sure probes are movable or removable. 
Feel a little hesitant to add a sampleFDO specific param to a common used interface because people may not understand when the param should be true or when it should be false. Even using the default version sometimes can block optimizations unintentionally in sampleFDO mode, like what the change tries to fix for taildup or branchfolding. 

pseudo hook itself has some ambiguity here. When compiler is trying to merge blocks with different pseudo hook, they are essential instructions which could affect the profile result. When compiler is trying to remove or thread empty block, pseudo hook is non essential. Could you give some actionable guidance as comments about when SkipPseudoOp should be true?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97481