[PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

On Wed, Feb 3, 2016 at 12:40 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
> On Feb 3, 2016, at 12:23 PM, Xinliang David Li <davidxl at google.com> wrote:
>
> On Tue, Feb 2, 2016 at 1:31 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
>
> On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
>
> silvas added a comment.
>
> In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
>
> For the longer term, one possible solution is to make FE based
> instrumentation only used for coverage testing which can be turned on
> with -fcoverage-mapping option (currently, -fcoverage-mapping can not
> be used alone and must be used together with
> -fprofile-instr-generate). To summarize:
>
> A. Current behavior:
>
> -----------------------
>
> 1. -fprofile-instr-generate turns on FE based instrumentation
> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based
> instrumentation and coverage mapping data generation.
> 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.
>
> B. Proposed new behavior:
>
> --------------------------------
>
> 1. -fprofile-instr-generate turns on IR late instrumentation
> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
> 4. -fprofile-instr-use=<> will automatically determine how to use the
> profile data.
>
>
>
> Very good observation that we can determine FE or IR automatically based on
> the input profdata. That simplifies things.
>
> B.2) above can be done today for improved usability.
>
>
>
> I don't see how this improves usability. In fact, it is confusing because it
> hijacks an existing option.
>
>
> Hijacking an existing option to do something different would definitely be a
> problem. Please find a way to specify IR-level instrumentation without
> breaking compatibility. If you want to replace the existing options with
> something different, we’ll need a transition period of at least 1-2 LLVM
> releases to migrate.
>
>
> If we remove B.3 above,  then the proposed change (B.2) is essentially
> making '-fcoverage-mapping' an alias to '-fprofile-instr-generate
> -fcoverage-mapping'.   No existing workflow will be broken and new
> users can take advantage of the shortened option.  The reason is that
> there will be no users that only use -fcoverage-mapping option alone
> and rely on its behavior (which is clang error).
>
>
> The part I’m concerned about is B.1. The current behavior is that
> -fprofile-instr-generate enabled FE instrumentation. We can’t hijack that to
> do something different, at least without a sufficiently long transition
> period for clients to adapt. We use that to generate PGO profiles even when
> not using -fcoverage-mapping.

yes. I don't see this happening overnight. IR based instrumentation
also needs to get stablized and widely used before the switch can
happen.

>>
> Yes, that is a requirement for us. We need existing profdata to work with
> newer versions of clang (which is why IR-level instrumentation doesn’t work
> for us).
>
>
> profile-use can automatically detect FE based profile data and use it
> properly. The question is whether we have a need to support merging
> profiles from IR and FE instrumentation.
>
>
> I don’t think it makes sense to merge those. They seem like fundamentally
> different kinds of data. The “forward compatibility” requirement is about
> different versions of the FE-based profile data.


great -- one fewer thing to worry about.

>

>
> I want to understand how we can guarantee to support old (FE based)
> profiles with new compilers.  The region to counter id
> mapping/assignment depends on how the AST is generated by the frontend
> and how the AST is traversed. Do we have any guarantee that the new
> compiler can generate them in the same order? How is that enforced?
> The function structural hash generated may also be different (given
> the same source).
>
>
> The FE-based instrumentation uses a custom traversal of the ASTs so that we
> can control the order and make sure it doesn’t change. It still depends on
> the way the ASTs are generated but the AST nodes that are relevant for this
> are unlikely to change in ways that would affect the instrumentation. I
> would love to have a better way to enforce that.

My guess is that IR based instrumentation (which is CFG based) can
produce as stable profile as what FE based instrumentation (when it is
used in a conservative mode that does not require pre-inlining).  CFG
based checksum is also used to detect incompatible changes such that
old profile data can be kept live for a long time (while
gradually/slowly degrades in quality due to the increased number of
mismatches).    To get really stable profile, we need to used a source
location based representation (which sample based PGO uses).    In
other words, I don't see a big obstacle that prevent IR based
instrumentation from being usable  in such a workflow.

>
> The hashing scheme is specified in a way that does not tie it to the details
> of the compiler version. And, if we change the hashing, there is a design
> requirement that we maintain compatibility with the old hashing based on the
> version number in the profile data. That gives us a strong incentive to
> minimize changes to the hashing, but I’m not very happy with the current
> hashing, so I do hope we can introduce a new version soon.

This requires a version bump and backward compatibility can be kept
(similar to what we do for changing the naming scheme of file static
functions).

thanks,

David


>
>
> thanks,
>
> David
>
>
>
>
>
>
>
> B.1) needs a
>
>
> transition period before  the IR based instrumentation becomes
>
>
> stablized (and can be flipped to the default).  During the transition
>
>
> period, the behavior of 1) does not change, but a cc1 option can be
>
>
> used to turn on IR instrumentation (as proposed by Sean).
>
>
>
> Just to clarify, users are not allowed to use cc1 options. The cc1 option is
> purely for us as compiler developers to do integration and testing, put off
> some discussions for later, etc.
>
>
> http://reviews.llvm.org/D15829
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>