[llvm-dev] The state of IRPGO (3 remaining work items)

[Sean Silva via llvm-dev]

On Thu, Jun 2, 2016 at 6:41 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Thu, Jun 2, 2016 at 5:30 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>>
>>
>> On Thu, Jun 2, 2016 at 2:51 PM, Frédéric Riss <friss at apple.com> wrote:
>>
>>>
>>> On Jun 2, 2016, at 12:10 AM, Sean Silva <chisophugis at gmail.com> wrote:
>>>
>>>
>>>
>>> On Wed, Jun 1, 2016 at 5:46 PM, Frédéric Riss <friss at apple.com> wrote:
>>>
>>>>
>>>> On Jun 1, 2016, at 1:46 PM, Sean Silva <chisophugis at gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On Tue, May 31, 2016 at 6:02 PM, Frédéric Riss <friss at apple.com> wrote:
>>>>
>>>>>
>>>>> On May 24, 2016, at 5:21 PM, Sean Silva <chisophugis at gmail.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Tue, May 24, 2016 at 3:41 PM, Vedant Kumar <vsk at apple.com> wrote:
>>>>>
>>>>>>
>>>>>> > On May 23, 2016, at 8:56 PM, Xinliang David Li <davidxl at google.com>
>>>>>> wrote:
>>>>>> >
>>>>>> > On Mon, May 23, 2016 at 8:23 PM, Sean Silva <chisophugis at gmail.com>
>>>>>> wrote:
>>>>>> > Jake and I have been integrating IRPGO on PS4, and we've identified
>>>>>> 3 remaining work items.
>>>>>> >
>>>>>> > Sean, thanks for the write up. It matches very well with what we
>>>>>> think as well.
>>>>>>
>>>>>> + 1
>>>>>>
>>>>>>
>>>>>> > - Driver changes
>>>>>> >
>>>>>> > We'd like to make IRPGO the default on PS4. We also think that it
>>>>>> would be beneficial to make IRPGO the default PGO on all platforms
>>>>>> (coverage would continue to use FE instr as it does currently, of course).
>>>>>> In previous conversations (e.g. http://reviews.llvm.org/D15829) it
>>>>>> has come up that Apple have requirements that would prevent them from
>>>>>> moving to IRPGO as the default PGO, at least without a deprecation period
>>>>>> of one or two releases.
>>>>>>
>>>>>> Sean pointed out the problematic scenario in D15829 (in plan "C"):
>>>>>>
>>>>>> ```
>>>>>> All existing user workflows continue to work, except for workflows
>>>>>> that attempt to llvm-profdata merge some old frontend profile data (e.g.
>>>>>> they have checked-in to version control and represents some special
>>>>>> workload) with the profile data from new binaries.
>>>>>> ```
>>>>>>
>>>>>> We can address this issue by (1) making sure llvm-profdata emits a
>>>>>> helpful warning when merging an FE-based profile with an IR-based one, and
>>>>>> (2) keeping an option to use FE instrumentation for PGO. Having (2) helps
>>>>>> people who can't (or don't want) to switch to IR PGO.
>>>>>>
>>>>>>
>>>>>> > I'd like to get consensus on a path forward.
>>>>>> > As a point of discussion, how about we make IRPGO the default on
>>>>>> all platforms except Apple platforms.
>>>>>>
>>>>>> I'd really rather not introduce this inconsistency. I'm worried that
>>>>>> it might lead to Darwin becoming a second-tier platform for PGO.
>>>>>>
>>>>>> Fred (CC'd) is following up with some of our internal users to check
>>>>>> if we can change the default behavior of -fprofile-instr-generate. He
>>>>>> should be able to chime in on this soon.
>>>>>>
>>>>>
>>>>> Sorry it took me so long.
>>>>>
>>>>
>>>> Hi Fred,
>>>>
>>>> My understanding is that you were specifically investigating whether
>>>> Apple needed compatibility for merging indexed profiles. Is that
>>>> compatibility needed? The only compelling argument I have heard to continue
>>>> to expose FEPGO is that Apple may have a compatibility requirement for
>>>> merging indexed profiles from previous compiler versions.
>>>>
>>>>
>>>> Sorry no, my comment had nothing to do with merging profiles. I
>>>> understand that this will break, and it might very well be an issue for us,
>>>> but I think there is a more fundamental issue with the proposed plan. As
>>>> you bring it up though, this is a user visible breakage that shouldn’t be
>>>> disregarded completely.
>>>>
>>>
>>> Merging with existing indexed profiles is the only user-visible breakage
>>> AFAIK (this was discussed at length in http://reviews.llvm.org/D15829 and
>>> the corresponding email thread). Please provide concrete examples where
>>> things would break.
>>>
>>>
>>> I feel like we are talking past each other. It is a fact that “merging
>>> existing indexed profiles” will break. You consider it to be a reasonable
>>> breakage, I’m just saying that we shouldn't ignore this completely. It is a
>>> known breakage so it should be considered when making the decision about
>>> the new flags’ behavior.
>>> We advise users to commit their profiles to VCS, so this could actually
>>> be a real issue for them (Xcode won’t try to merge old/new profiles by
>>> itself though, but users could do this). For those kind of changes where
>>> the alternative is not a full drop-in replacement, I think deprecating the
>>> option and replacing it with a new one is a better methodology.
>>>
>>>
>>>
>>>>
>>>> Even if this is a requirement, then I still intend to make IRPGO the
>>>> default and only PGO going forward, at least on PS4. I think that doing the
>>>> same for all platforms in the upstream compiler probably makes sense as
>>>> well, since an internal Apple vendor compatibility requirement should not
>>>> penalize all users of the open source project.
>>>>
>>>>
>>>> Again, I’m not expressing an Apple requirement, just trying to discuss
>>>> the specifics of the proposed implementation. My goal is not to hinder
>>>> anything, and I want our platforms to be able to use IRPGO reliably if
>>>> users see the need for it.
>>>>
>>>
>>> What I'm saying is that besides reduced training overhead (and the
>>> inability to merge with older indexed profiles, which AFAIK is the only
>>> actual potential requirement that would need a deprecation period for
>>> FEPGO), IRPGO is basically "just a better PGO", so adding a frontend one
>>> (except as something purely during a deprecation period) is
>>> pointless. "just a better PGO" is what IRPGO is for my users. I don't want
>>> to have to have them deal with (and I don't want to support) FEPGO.
>>>
>>>
>>> Well we want to support FEPGO because it fits Xcode's workflow better
>>> (mainly it allows to do PGO + coverage at the same time).
>>>
>>> Anything that will cause the existing flag to continue to produce FEPGO
>>> on PS4 is not something that I'm really okay with. The reduced overhead of
>>> IRPGO is really important on PS4 (i.e. the difference between the
>>> instrumented game being playable or not). I really don't want to have to
>>> test the triple to determine the meaning of `-fprofile-instr-generate`
>>> (without `-fcoverage-mapping`).
>>>
>>>
>>> And I agree that IRPGO is better for your users, but the best workflow
>>> for your users is not necessarily the best workflow for every clang user. I
>>> totally agree with you that testing the triple to decide the meaning of the
>>> option is wrong to do upstream. The best default is not a platform choice,
>>> it’s a workflow decision.
>>>
>>>
>>>> I’ve discussed the change in behavior quiet extensively, and I after
>>>>> having changed my mind a couple times, I would argue in favor of keeping
>>>>> the current behavior for the existing flags. I think adding a new switch
>>>>> for IRPGO is a better option. The argument that weighted most on my opinion
>>>>> is the proposed interaction with -fcoverage-mapping, and it is not at all
>>>>> platform specific. With the proposed new behavior, turning coverage on and
>>>>> off in your build system will generate a binary with different performance
>>>>> characteristics and this feels really wrong.
>>>>>
>>>>
>>>> Bob already mentioned in the other thread that
>>>> `-fprofile-instr-generate -fcoverage-mapping` was sufficiently different
>>>> from `-fprofile-instr-generate` that `-fprofile-instr-generate
>>>> -fcoverage-mapping` was not an acceptable workaround that could be used for
>>>> enabling FEPGO during a transitionary period, so I'm not convinced that
>>>> your argument here makes sense.
>>>>
>>>>
>>>> I’m not sure what you’re referring to here, and I have a hard time
>>>> parsing the sentence. I suppose “was not an acceptable” should read “was an
>>>> acceptable”? I would be surprised that Bob ever agreed to completely
>>>> transition away from FEPGO. I didn’t even understand that getting rid of
>>>> FEPGO was on table as you seem to imply bellow.
>>>>
>>>
>>> No, it is written as intended. The backstory is in
>>> http://reviews.llvm.org/D15829 (and the corresponding email thread).
>>> The paragraph starting with "The coverage mapping adds considerable cost.”.
>>>
>>>
>>> Thanks for the pointer, it turns out this part of the thread never made
>>> it to my mailbox. What Bob is saying is that we need an option to turn on
>>> FEPGO without coverage, and this is basically exactly what I think we
>>> should do: add an explicit option to choose the instrumentation type, so
>>> that we can use FEPGO even without coverage in the new IRPGO world.
>>>
>>>
>>>> I also share David's opinion that this is not going to be an issue in
>>>> practice. I think it makes sense for PGO and coverage to have different
>>>> overheads. Coverage inherently has to trace all locations at source level,
>>>> while PGO has more freedom.
>>>>
>>>>
>>>> I’m sorry if I wasn’t clear, but I’m not talking about instrumentation
>>>> overhead, I’m talking about the performance of the binary generated using
>>>> the profiles. If we go the route of making the meaning of
>>>> -fprofile-instr-generate depend on whether -fcoverage-mapping gets passed,
>>>> then we change the kind of instrumentation and thus the input to the
>>>> optimizations behind the user’s back. I wouldn’t be surprised that using
>>>> profiles generated by FEPGO and IRPGO give you a final executable with
>>>> measurably different performance characteristics.
>>>>
>>>
>>> I think the point is that given the effort being put into IRPGO, the
>>> IRPGO version will always be a faster final executable.
>>>
>>>
>>> 2 things:
>>>  - Do you have strong evidence of this being true in all cases? I’m not
>>> looking forward to when I’ll transition Apple’s clang to IRPGO, because I
>>> know that we’ll have to spend time characterizing the performance of the
>>> compiler. It might be faster in all but a couple benchmarks, we still will
>>> need to investigate what happened there to see if we can fix it. I’m not
>>> saying it’s bad (I’m actually also looking forward to evaluate it, because
>>> it might give better results for us), but it’s definitely not a benign
>>> change that I would do without double checking the results. And as such, I
>>> also wouldn’t want the compiler to silently do this change behind my back.
>>>  - Even if it is 100% true, then users will see a degradation in
>>> performance if they add -fcoverage-mapping to their CFLAGS. I think this is
>>> just bad user experience. The user should be able to decide if he wants to
>>> be able to compromise some performance to be able to do PGO+coverage at the
>>> same time.
>>>
>>> Why provide a "worse" PGO option?
>>>
>>>
>>> Worse from your point of view. The resilience to compiler changes that
>>> FEPGO currently offers is a real feature even if it doesn’t have a place in
>>> Sony’s workflow.
>>>
>>>
>>>
>>>> If you’re tracking your performance, this can be really painful.
>>>> Recently we wasted days investigating performance regressions that were due
>>>> to buggy profiles. I strongly believe having an option seemingly unrelated
>>>> to PGO change this behavior is wrong and can cause actual pain for our end
>>>> users.
>>>>
>>>
>>> After a deprecation period we can force `-fprofile-instr-generate` and
>>> `-fcoverage-mapping` to be mutually exclusive if necessary. Does this solve
>>> your problem?
>>>
>>>
>>> We don’t want a “deprecation period”. Unless IRPGO evolves in something
>>> that can do precise coverage and PGO in the same run, FEPGO can’t be
>>> deprecated (as in slated for removal).
>>>
>>> Actually, I think it makes a lot of sense in some respects for
>>> `-fprofile-instr-generate` and `-fprofile-instr-generate
>>> -fcoverage-mapping` to be IRPGO and FEPGO/coverage. The difference from a
>>> user's perspective is basically "is the instrumentation inserted by the
>>> compiler constrained to have source-level coverage, or does the compiler
>>> not have this restriction". Although as I've said, I'm not a fan of
>>> supporting FEPGO in the long-term due to maintenance issues.
>>>
>>> Also note that things like the context-sensitivity obtained through
>>> pre-inlining (see Rong's original RFC) is simply not obtainable within a
>>> source-level instrumentation paradigm (even if we did something like the
>>> counter fusion discussed in "[llvm-dev] RFC: Pass to prune redundant
>>> profiling instrumentation" to reduce the overhead to that of IRPGO with
>>> pre-inlining). Thus FEPGO a.k.a. "coverage-level PGO" would nonetheless be
>>> at an inherent disadvantage.
>>>
>>>
>>>> Also, David's point about redundant work on FEPGO is a good one. We
>>>> don't want to continue maintaining two different PGO’s.
>>>>
>>>>
>>>> Are you implying that LLVM should drop FEPGO? It’s a totally sensible
>>>> thing to do to use your tests as training data for your profile generation.
>>>> It’s also a very valid thing to do to use your tests to do coverage. Xcode
>>>> does both of these things. I would see it a a big regression to not support
>>>> doing both at the same time (this would mean doubling compile+testing time
>>>> for users of both).
>>>>
>>>
>>> As David pointed out, training runs for PGO and coverage have different
>>> goals. I'm very skeptical of any testing that tries to do both at the same
>>> time, but this will continue to work (albeit without benefitting from any
>>> of the effort being put into IRPGO).
>>>
>>>
>>> For a high-end game, sure. Small apps are a different beast and it’s a
>>> reasonable thing to use your set of tests as training data. Again, all the
>>> workflows are not equivalent.
>>>
>>> As the instrumentation needs to stay there for coverage anyway, I expect
>>>> FEPGO to stay there and maintained (we care a lot about coverage). I’m not
>>>> saying that all the work going into IRPGO should be duplicated in FEPGO,
>>>> but what’s there and working should keep working.
>>>>
>>>
>>> For my users the reduced overhead of IRPGO is an important feature, and
>>> making it the default is important for that reason. Since most of the
>>> effort going into PGO is focused on IRPGO, this will lead to users using
>>> FEPGO ending up as a second-tier PGO, which Vedant said he specifically
>>> wanted to avoid. The only option to avoid this is for users to not be using
>>> FEPGO.
>>>
>>>
>>> I’d love to have everybody be able to enjoy the lower overhead, but if
>>> the answer to this is that they need to compile+train twice to get coverage
>>> and PGO, then the trade-off is wrong. I suppose you can agree that every
>>> user doesn’t have the same requirements as game developers. At least users
>>> should have the choice.
>>>
>>> Also, FEPGO has a lot of nice characteristics like resilience to IRGEN
>>>> changes. If you have archived profiles, then when you switch compilers your
>>>> performance shouldn’t degrade with FEPGO (modulo optimization bugs), while
>>>> it’s much more likely to degrade with IRPGO.
>>>>
>>>
>>> Note that this use case continues to work. I.e. we continue to apply
>>> existing frontend profiles correctly. including frontend profiles generated
>>> with -fcoverage-mapping, so that collecting coverage and PGO at the same
>>> time (although not advisable) still works. The only use case that breaks is
>>> merging existing indexed profiles, which is why we are specifically waiting
>>> for an answer on whether this is a requirement for you guys at Apple, which
>>> will determine what kind of deprecation period etc. will be needed before
>>> we can default it.
>>>
>>>
>>>> It overall looks like a much better option for people who do not need
>>>> the lower instrumentation overhead.
>>>>
>>>
>>> This is not just about lower instrumentation overhead. Things like the
>>> recently added static VP node allocation (which will e.g. make indirect
>>> callsite promotion for LTO'd kernels work) are other things are being
>>> missed out on.
>>>
>>>
>>>
>>>> I would actually make the IRPGO mode completely incompatible with the
>>>>> -fcoverage-mapping flag.
>>>>>
>>>>
>>>> I'm not sure what you mean by this. Nobody is proposing anything that
>>>> would make -fcoverage-mapping do anything related to IRPGO.
>>>>
>>>>
>>>> What I mean is that -f<whatever enables IRPGO> should error out when
>>>> passed at the same time as -fcoverage-mapping.
>>>>
>>>
>>> I think you're coming into this with the mindset that FEPGO will still
>>> be a possibility (outside of a build that is used for coverage mapping).
>>> I'm not convinced that we actually need to continue exposing that except as
>>> a weird thing in conjunction with coverage (and possibly for a deprecation
>>> period if users want to merge indexed profiles).
>>>
>>>
>>> FEPGO needs to be a possibility. And the instrumentation code needs to
>>> be there anyway for the coverage. Here’s a proposal:
>>>  - Add -fpgo-instr=[llvm|clang] to choose which kind of PGO
>>> instrumentation you want
>>>  - make -fpgo-instr=llvm incompatible with with -fcoverage-mapping
>>>  - deprecate -fprofile-instr-generate, and remove it after a couple
>>> releases
>>>
>>> (I really don’t care about the names)
>>>
>>> I think it is a better way forward to have -fprofile-instr-generate keep
>>> it’s current meaning and to have downstream toolchain providers have an
>>> internal patch to make it an alias to the one they prefer. Just because
>>> changing the meaning of options is a bad thing.
>>>
>>
>> Sure, I can deal with that. And actually this discussion has suggested
>> some good user-understandable names for the two different PGO's. I think
>> that "stable PGO" or "coverage-based PGO" or "source-level PGO" is a really
>> good name for FEPGO. "frontend" and "IR" aren't really meaningful to users.
>>
>> Based on what I have seen, FEPGO has the following reasons to keep it
>> around:
>> - it currently can be used together with coverage
>> - it has certain compatibility guarantees about profiles across compiler
>> versions
>>
>>
>>
>>> This also means that if the consensus is that -fprofile-instr-generate
>>> should really change its meaning to mean IRPGO, I’m open to having this
>>> internal patch on our side.
>>>
>>
>> Yeah, it sounds like someone is going to have to keep a "private patch"
>> to change the default. At that point doing a switch on the triple in
>> upstream seems preferable though :/
>>
>>
>> So I propose the following (which is equivalent to what you proposed, but
>> starting to put specific option names):
>> 1. Add -fprofile-instr-generate=stable and
>> -fprofile-instr-generate=unstable
>> a) Indexed profiles for -fprofile-instr-generate=stable are guaranteed to
>> be compatible across compiler releases (which will include existing
>> releases). Additionally, -fprofile-instr-generate=stable can be used in
>> combination with -fcoverage-mapping.
>> b) Indexed profiles for -fprofile-instr-generate=unstable are not
>> guaranteed to be compatible across compiler releases. It is an error to use
>> this in conjunction with -fcoverage-mapping
>> c) -fprofile-instr-generate defaults to one of the two in a way that
>> meets vendor needs, via private patches or an upstream switch on the triple
>> or whatever. It can eventually be deprecated or whatever.
>>
>> -fprofile-instr-generate=stable would basically correspond to FEPGO / FE
>> coverage instrumentation. -fprofile-instr-generate=unstable would basically
>> correspond to IRPGO.
>>
>> What do you think?
>>
>
>
> Sounds fine to me, though I am not a fan of using unstable in the option.
> I think a more meaningful way (that capture the essence of the difference)
> is the following naming:
> 1) FEPGO:  -fprofile-instr-generate=source or
> -fprofile-instr-generate=region
> 2) IR: -fprofile-instr-generate=cfg or -fprofile-instr-generate=graph
>
> Also since -fprofile-instr-generate= form is already used to specify raw
> profile path, we may need a different driver option.
>

Oops!


> Alternatives include
> 1) -fprofile-instrument=<...> -- this maps directly to the cc1 option we
> have today
> or
> 2) -fpgo-instr=<> -- suggested by Fred or
> 3) -fpgo-instr-method=<...>
>

I will probably use one of these in the patch.

-- Sean Silva


>
>
> Having the driver level option is the first good step forward.  In the
> near future, when performance of IRPGO is further tuned (e.g, better
> integration with inliner, more runtime perf win with -flto=thin, -flto
> etc), and the interests of the IRPGO is better aligned, we can revisit the
> default.
>
> thanks,
>
> David
>
>
>
>
>>
>> -- Sean Silva
>>
>>
>>>
>>> I also think a new option is cleaner (again I don’t care about the
>>> name), but if people feel that -fprofile-instr-generate=[llvm|clang] is
>>> easier, then this works for me too.
>>>
>>> Fred
>>>
>>>
>>> -- Sean Silva
>>>
>>>
>>>>
>>>> Thanks!
>>>> Fred
>>>>
>>>> -- Sean Silva
>>>>
>>>>
>>>>
>>>>>
>>>>> Fred
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> At its core I don't think -fprofile-instr-generate *implies* FE-based
>>>>>> instrumentation. So, I'd like to see the driver do this (on all platforms):
>>>>>>
>>>>>>   * -fprofile-instr-generate: IR instrumentation
>>>>>>   * -fprofile-instr-generate=IR: IR instrumentation
>>>>>>   * -fprofile-instr-generate=FE: FE instrumentation
>>>>>>   * -fprofile-instr-generate -fcoverage-mapping: FE + coverage
>>>>>> instrumentation
>>>>>>
>>>>>> It's a bit ugly because the meaning of -fprofile-instr-generate
>>>>>> becomes context-sensitive. But, (1) it doesn't break existing common
>>>>>> workflows and (2) it makes it easier to ship IRPGO. The big caveat here is
>>>>>> that we'll need to wait a bit and see if our internal users are OK with
>>>>>> this.
>>>>>>
>>>>>
>>>>> Is there a reason to even have the possibility for FEPGO in the long
>>>>> run? From what I can tell, at most we would add a
>>>>> -fuse-the-old-pgo-because-i-want-to-merge-with-old-profiles option to
>>>>> hold people over until they can regenerate their profiles with the current
>>>>> compiler. We can add a flag to control what pre-instrumentation is done to
>>>>> retain the source-level robustness of FEPGO (e.g.
>>>>> -fpgo-no-simplify-before-instrumenting or something).
>>>>>
>>>>>
>>>>>> One alternative is to introduce a separate driver flag for IRPGO.
>>>>>> This might not work well for Sony's existing users. I'd be interested in
>>>>>> any feedback about this approach.
>>>>>>
>>>>>
>>>>> Personally, I would prefer to maintaining command line compatibility
>>>>> for PGO in Clang (i.e. users don't have to modify their build systems).
>>>>>
>>>>>
>>>>> -- Sean Silva
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> > I really don't like fragmenting things like this (e.g. if a
>>>>>> third-party tests "clang's" PGO they will get something different depending
>>>>>> on the platform), but I don't see another way given Apple's constraints.
>>>>>> >
>>>>>> > I'd like to see IRPGO to be the default as well, but the first
>>>>>> thing we need is a driver level option to make the switch (prof-gen) --
>>>>>> currently we rely on -Xclang option to switch between two modes, which is
>>>>>> less than ideal.
>>>>>> >
>>>>>> > If the concern from Apple is that the old profile still need to
>>>>>> work, then this is problem already solved. The reason is that
>>>>>> -fprofile-instr-use can automatically detect the type of the profile and
>>>>>> switch the mode.
>>>>>>
>>>>>> It's not just that. As Sean pointed out, we're concerned about old
>>>>>> profiles inter-operating poorly with new ones.
>>>>>>
>>>>>> thanks,
>>>>>> vedant
>>>>>>
>>>>>>
>>>>>> > - Pre-instrumentation passes
>>>>>> >
>>>>>> > Pre-instrumentation optimization has been critical for reducing the
>>>>>> overhead of PGO for the PS4 games we tested (as expected). However, in our
>>>>>> measurements (and we are glad to provide more info) the main benefit was
>>>>>> inlining (also as expected). A simple pass of inlining at threshold 100
>>>>>> appeared to give all the benefits. Even inlining at threshold 0 gave almost
>>>>>> all the benefits. For example, the passes initially proposed in
>>>>>> http://reviews.llvm.org/D15828did not improve over just inlining
>>>>>> with threshold 100.
>>>>>> >
>>>>>> > (due to PR27299 we also need to add simplifycfg after inlining to
>>>>>> clean up, but this doesn't affect the instrumentation overhead in our
>>>>>> measurements)
>>>>>> >
>>>>>> > Bottom line: for our use cases, inlining does all the work, but
>>>>>> we're not opposed to having more passes, which might be beneficial for
>>>>>> non-game workloads (which is most code).
>>>>>> >
>>>>>> >
>>>>>> >
>>>>>> > Yes, Rong is re-collecting performance data before submitting the
>>>>>> patch.
>>>>>> >
>>>>>> > - Warnings
>>>>>> >
>>>>>> > We identified 3 classes of issues which manifest as spammy warnings
>>>>>> when applying profile data with IRPGO (these affect FEPGO also I believe,
>>>>>> but we looked in depth at IRPGO):
>>>>>> >
>>>>>> > 1. The main concerning one is that getPGOFuncName mangles the
>>>>>> filename into the counter name. This causes us to get
>>>>>> instrprof_error::unknown_function when the pgo-use build is done in a
>>>>>> different build directory from the training build (which is a reasonable
>>>>>> thing to support). In this situation, PGO data is useless for all `static`
>>>>>> functions (and as a byproduct results in a huge volume of warnings).
>>>>>> >
>>>>>> > This can be enhanced with an user option to override the behavior.
>>>>>> Can you help filing a tracking bug?
>>>>>> >
>>>>>> >
>>>>>> > 2. In different TU's, pre-instr inlining might make different
>>>>>> inlining decisions (for example, different functions may be available for
>>>>>> inlining), causing hash mismatch errors (instrprof_error::hash_mismatch).
>>>>>> In building a large game, we only saw 8 instance of this, so it is not as
>>>>>> severe as 1, but would be good to fix.
>>>>>> >
>>>>>> >
>>>>>> > Rong has a patch addressing that -- will submit after cleanup pass
>>>>>> change is done.
>>>>>> >
>>>>>> >
>>>>>> > 3. A .cpp file may be compiled and put into an archive, but then
>>>>>> not selected by the linker and will therefore not result in a counter in
>>>>>> the profraw. When compiling this file with pgo-use,
>>>>>> instrprof_error::unknown_function will result and a warning will be emitted.
>>>>>> >
>>>>>> > yes -- this is a common problem to other compilers as well.
>>>>>> >
>>>>>> >
>>>>>> >
>>>>>> > Case 1 can be fixed using a function hash or other unique
>>>>>> identifier instead of a file path. David, in D20195 you mentioned that Rong
>>>>>> was working on a patch that would fix 2; we are looking forward to that.
>>>>>> >
>>>>>> >
>>>>>> > Right.
>>>>>> >
>>>>>> > For 3, I unfortunately do not know of any solution. I don't think
>>>>>> there is a way for us to make this warning reliable in the face of this
>>>>>> circumstance. So my conclusion is that instrprof_error::unknown_function at
>>>>>> least must be defaulted to off unfortunately.
>>>>>> >
>>>>>> > yes, this can be annoying. If the warnings can be buffered, then
>>>>>> the compiler can check if this is due to missing profile for the whole file
>>>>>> and can reduce the warnings into one single warning (source file has no
>>>>>> profile data).  Making it off by default sounds fine to me too if it is too
>>>>>> noisy.
>>>>>> >
>>>>>> > thanks,
>>>>>> >
>>>>>> > David
>>>>>> >
>>>>>> >
>>>>>> > -- Sean Silva
>>>>>>
>>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160603/d745790a/attachment-0001.html>