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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
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 {
----------------
wmi wrote:
> wmi wrote:
> > 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?
> > 
> Think about it a little more. If tail merge (merge blocks that look the same except for their pseudo probes) is causing trouble for pseudo hook based profile accuracy, is it possible to just directly turn off tail merge if pseudo hook is in use? In that way, we eliminate the ambiguity of pseudo hook and we can always treat it the same way as debug instructions in getFirstNonDebugInstr.
That's a valid concern. Yeah, the semantics of pseudo probes as far as whether optimizations should be blocked is ambiguous and requires user to be judicious. We are trying to make a trade off between profile quality and code quality. I went through all the uses of these APIs and changed some of them to skip probes for better quality while not damaging profile quality. I'll put some comments based on my experience.

Regarding tail merge, sometimes it is useful to blocks with exactly the same code including probes. A pass typically does not handle a single pattern like block merge or block duplicate. For example, both the merge and duplicate is done by the jump threading pass. Disabling a part of a pass (especially a future pass) may not be done automatically and still requires user's judgement.

I agree that letting optimization developer be aware of sample profile quality may not be practical. So far the approach I'm taking is to keep a good profile quality while fixing code quality regression. I expect at some time we can flip over the default value of `SkipPseudoOp` for maximum code quality and we keep an eye on the profile quality. We can also make a knob that automatically changes the default value to favor either factor. 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97481