[PATCH] D148876: [IndirectCallPromotion] Clear value profile metadata after the last run of indirect-call-promotion in a pipeline

[Mingming Liu via Phabricator via llvm-commits]

mingmingl added a comment.

In D148876#4297923 <https://reviews.llvm.org/D148876#4297923>, @wenlei wrote:

> In D148876#4288429 <https://reviews.llvm.org/D148876#4288429>, @wenlei wrote:
>
>> cc @hoy to make sure this works for csspgo.
>
> Sorry for being vague. What I meant is to double check the pass ordering to make sure csspgo doesn't have extra use of VP after sample loader and ICP, so clearing is ok.
>
> CGProfile is a good catch. Now if the final solution is to allow merging of VP after ICP is done, instead clearing all VP, I don't see any problem then.

Yep without the catch the clearing option would be bad. So indeed good catch!

As an update, I'll proceed to implement the idea of allow merging after ICP transformation, with slightly better implementations than extending "vp" format;

1. given `allow-merging` is either true or false for all value-profile instances depending on the execution stage of pipeline, use per module flag rather than per `!prof` boolean
2. If possible, introduce helper functions for individual transformation passes (simplifycfg or cse or whatever pass) to call, the helper function either returns  'wait, don't merge for now' (maybe fleshes out as `nullptr`) or the result of the merged instructions (with merged value profiles).

This way, 1) reduces repeated states to a per-module level, and 2) makes it easier to preserve metadata for other passes (not only simplify-cfg, might be early-cse pass, etc).

In D148876#4297926 <https://reviews.llvm.org/D148876#4297926>, @wenlei wrote:

> In D148876#4288504 <https://reviews.llvm.org/D148876#4288504>, @mingmingl wrote:
>
>> In D148876#4288286 <https://reviews.llvm.org/D148876#4288286>, @tejohnson wrote:
>>
>>> Would be good to explain motivation (maybe reference other patch?)
>>
>> Makes sense, will do.
>
> Can I piggyback on this to suggest a general feedback :) Reviewers would benefit from more explanation on why, in addition to what, and it applies to other patches as well. Without asking explicitly, I failed to grasp your intention of D147954 <https://reviews.llvm.org/D147954> just by looking at the patch without asking questions. It somewhat applies to the profile reader changes too.

Sure, I'll try to do better. My take is that natural language could be ambiguous compared with reduced godbolt examples (or diagrams to describe cfg, etc). Even with how-and-why explained, I could imagine questions are welcome rather than being frowned upon, like how existing questions inspires me to learn and find problems early.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148876