[PATCH] D107034: [CSPGO] Set EnableValueProfiling flag for CSPGO to avoid IRPGO value loss in LTO

Fri Aug 20 12:38:56 PDT 2021

MaskRay added a comment.

In D107034#2956812 <https://reviews.llvm.org/D107034#2956812>, @YolandaCY wrote:

> In D107034#2956730 <https://reviews.llvm.org/D107034#2956730>, @MaskRay wrote:
>
>> About this patch `M.addModuleFlag(llvm::Module::Warning, "EnableValueProfiling", 1);`: I am not so sure we should do it.
>
> From `enablesValueProfiling` both IRPGOFlag and EnableValueProfiling mudule flag are examined as indicators. My understanding IRPGOFlag should imply "EnableValueProfiling" to 1.
>
> This alternative solution was recommended by Reid, and no objection from Xu. Could you help clarify with Fangrui? @rnk @xur

`"EnableValueProfiling"` is set to 0 by -fprofile-instr-generate to enable a size saving optimization (D102818 <https://reviews.llvm.org/D102818>).

IR profiling doesn't set 1. It is inconsistent for CSPGO to set `"EnableValueProfiling"` to 1, while -fprofile-generate keeps omitting the module flag.
`-disable-vp` can disable value profiling. This patch doesn't respect that option.

In addition, I think changing the condition in `getVarName` is incorrect. Coverage and PGO have different use cases while share some infrastructure (InstrProfiling.cpp).
Coverage suppresses renaming to have better coverage results. The decision is independent of whether value profiling is enabled or not.
On the other hand, PGO's preferring renaming behavior is also independent of value profiling.

Some confusion here is that `-fprofile-instr-generate` was used to do PGO (perhaps all groups except Apple have migrated away).
When `"EnableValueProfiling"` was introduced, I think the intention was to prevent breakage to them.
Otherwise, PGO use cases could just ignore `"EnableValueProfiling"` everywhere.

Actually, I am thinking of dropping `"EnableValueProfiling"`. It needs some COFF side work (D44543 <https://reviews.llvm.org/D44543>) (or we just unsupport that use case).

>> Note that we only set "EnableValueProfiling" to 0 for the `clang -fprofile-instr-generate` case. The value is not set to 1 (without poking into the internal `-enable-value-profiling` option).
>>
>> `clang -fprofile-generate` with value profiling doesn't set this module flag, so -fcs-profile-generate doesn't need to set it, either.
>
> I think I have clarified why this change is unnecessary to PGO in previous reply.
>
> I can drop the unrelated change regarding /lldsavetemps for sure. That should be an easy fix.

As previously explained, I think the patch is not correct. So keeping the "Request Changes" state.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107034