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

[Sean Silva via llvm-dev]

Quick update. I've gotten derailed from posting a patch for this due to
focusing on higher priority PGO inlining work. No ETA.

-- Sean Silva

On Fri, Jun 3, 2016 at 6:06 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> 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/20160613/f257e341/attachment-0001.html>