[PATCH] Improve performance of calculateDbgValueHistory
Alexey Samsonov
vonosmas at gmail.com
Wed Aug 6 16:37:39 PDT 2014
Thanks for the improvement! (and sorry that I wasn't able to review this
patch yesterday).
On Wed, Aug 6, 2014 at 11:50 AM, Adrian Prantl <aprantl at apple.com> wrote:
> Committed as r214987.
>
> -- adrian
>
> > On Aug 6, 2014, at 10:02 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > On Wed, Aug 6, 2014 at 9:35 AM, Adrian Prantl <aprantl at apple.com> wrote:
> >> I have one final nitpick; we should probably describe the status quo
> without referring to old versions of the implementation. E.g., instead of
> >>
> >> // The old version of this function just collected the registers in a
> std:set,
> >> // but the construction/iteration/destruction of these all sets had a
> significative
> >> // impact on debug info emission time. The functor version allows us to
> use
> >> // lambdas that don't need the temporary std::sets.
> >>
> >> we could write something like
> >>
> >> // By using a functor instead of a std::set& here, we can avoid the
> overhead of
> >> // constructing temporaries, which has a significant performance impact.
> >
> > Agreed - though perhaps with reference to the caller that needs to
> > construct a std::set? (when I first looked at this patch I looked at
> > the first caller which didn't gain any improvement by this change & I
> > was confused - but the second caller was able to eliminate a temporary
> > std::set which is where the win was)
> >
> > I'm OK with the patch otherwise. (In the future, something like that
> > comment fix (which was a correction even for the code as-is, without
> > your change) could be committed separately and without review)
> >
> > This seems clear enough that I'm happy signing off & letting Alexey
> > follow up in post-commit if he has anything to add,
> >
> > - David
> >
> >>
> >> -- adrian
> >>
> >>> On Aug 6, 2014, at 7:47 AM, Eric Christopher <echristo at gmail.com>
> wrote:
> >>>
> >>> Seems reasonable to me. Dave had been looking at it so let him give
> >>> the final OK :)
> >>>
> >>> -eric
> >>>
> >>> On Tue, Aug 5, 2014 at 11:14 PM, Frédéric Riss <friss at apple.com>
> wrote:
> >>>> Here’s the patch without the stylistic modifications. I kept the
> comment
> >>>> correction as I’m touching the corresponding function in the patch.
> I’ll
> >>>> submit the cleanup patch separately.
> >>>>
> >>>> OK to commit?
> >>>>
> >>>>
> >>>>
> >>>> Fred
> >>>>
> >>>>
> >>>> On 05 Aug 2014, at 21:35, David Blaikie <dblaikie at gmail.com> wrote:
> >>>>
> >>>> Best to split it out at least and we can review it separately. Thanks!
> >>>>
> >>>> (While this may sound like it creates a lot of busy work, reviewing
> smaller
> >>>> patches is exponentially easier so can result in faster turn around
> :))
> >>>>
> >>>> On Aug 5, 2014 12:19 PM, "Frédéric Riss" <friss at apple.com> wrote:
> >>>>>
> >>>>> Oh, you’re right, this hunk isn’t explained. It’s just a general
> cleanup
> >>>>> that I did while reading the code. If I’m not mistaken, the
> IsInEpilogue
> >>>>> removal is functionally equivalent to the old version. I just used
> an early
> >>>>> exit instead of continuing iterating the loop with a true
> IsInEpilogue
> >>>>> variable. I also added !MI.isDebugValue() as this function checks
> for MIs
> >>>>> that clobber registers, which can’t be the case of DebugValues I
> think. I
> >>>>> also changed the comment to reflect what the function really does.
> >>>>>
> >>>>> This can be split out in a separate cleanup patch if needed, or
> dropped
> >>>>> entirely if I missed something…
> >>>>>
> >>>>> Fred
> >>>>>
> >>>>> On 05 Aug 2014, at 21:03, David Blaikie <dblaikie at gmail.com> wrote:
> >>>>>
> >>>>>> (+Alexey who made the recent changes)
> >>>>>>
> >>>>>> Frédéric - This looks like general goodness except I'm a bit
> confused
> >>>>>> about the other changes you made to collectChangingRegs (removing
> >>>>>> IsEpilogue and associated changes). What was the purpose of those
> >>>>>> changes? Could they be separated from this commit to keep the change
> >>>>>> simpler and so it has a clear, singular purpose?
> >>>>>>
> >>>>>> - David
> >>>>>>
> >>>>>> On Tue, Aug 5, 2014 at 11:33 AM, Frédéric Riss <friss at apple.com>
> wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> In r210492 the logic of calculateDbgValueHistory was improved to
> end
> >>>>>>> register variable live ranges at the end of MBB conditionally on
> the fact
> >>>>>>> that the register was or not clobbered by the function body. This
> requires 2
> >>>>>>> passes over all the operands of the function. The first pass
> computes the
> >>>>>>> std::set of all clobbered registers of the function and in the
> second one we
> >>>>>>> intersect the global set with the one recomputed for the current
> >>>>>>> Instruction.
> >>>>>>>
> >>>>>>> The logic introduced in r210492 is necessarily a bit slower than
> before
> >>>>>>> as it involves walking twice the full list of MachineOperands for
> the
> >>>>>>> function. The new information is necessary, but the performance of
> debug
> >>>>>>> info emission is degraded by more than 10% on some benchmarks.
> >>>>>>>
> >>>>>>> The biggest performance hit is due to the
> >>>>>>> construction/iteration/destruction of std::set for each real
> >>>>>>> MachineInstruction in the second pass. We can avoid the temporary
> sets by
> >>>>>>> using a lambda that captures the global set and operates directly
> on that. I
> >>>>>>> tried various other approaches and this one gets the best
> performance while
> >>>>>>> still being quite readable.
> >>>>>>>
> >>>>>>> Here are some performance numbers (5 runs each time):
> >>>>>>>
> >>>>>>> Current mainstream
> >>>>>>> ---User Time--- --System Time-- --User+System-- ---Wall
> Time---
> >>>>>>> --- Name ---
> >>>>>>> 0.0508 ( 71.0%) 0.0262 ( 50.3%) 0.0770 ( 62.3%) 0.0772 (
> 62.3%)
> >>>>>>> Debug Info Emission
> >>>>>>> 0.0515 ( 71.1%) 0.0267 ( 50.7%) 0.0782 ( 62.5%) 0.0780 (
> 62.2%)
> >>>>>>> Debug Info Emission
> >>>>>>> 0.0506 ( 71.3%) 0.0262 ( 50.5%) 0.0768 ( 62.5%) 0.0768 (
> 62.5%)
> >>>>>>> Debug Info Emission
> >>>>>>> 0.0511 ( 70.6%) 0.0266 ( 50.5%) 0.0778 ( 62.1%) 0.0779 (
> 62.4%)
> >>>>>>> Debug Info Emission
> >>>>>>> 0.0507 ( 71.1%) 0.0261 ( 50.7%) 0.0769 ( 62.5%) 0.0768 (
> 62.5%)
> >>>>>>> Debug Info Emission
> >>>>>>>
> >>>>>>> With Patch
> >>>>>>> ---User Time--- --System Time-- --User+System-- ---Wall
> Time---
> >>>>>>> --- Name ---
> >>>>>>> 0.0380 ( 64.2%) 0.0259 ( 50.4%) 0.0639 ( 57.8%) 0.0635 (
> 57.7%)
> >>>>>>> Debug Info Emission
> >>>>>>> 0.0377 ( 63.7%) 0.0266 ( 50.5%) 0.0643 ( 57.5%) 0.0645 (
> 57.8%)
> >>>>>>> Debug Info Emission
> >>>>>>> 0.0383 ( 64.2%) 0.0264 ( 50.4%) 0.0647 ( 57.7%) 0.0651 (
> 58.1%)
> >>>>>>> Debug Info Emission
> >>>>>>> 0.0383 ( 63.7%) 0.0272 ( 50.8%) 0.0656 ( 57.6%) 0.0664 (
> 58.0%)
> >>>>>>> Debug Info Emission
> >>>>>>> 0.0376 ( 64.3%) 0.0261 ( 50.3%) 0.0637 ( 57.7%) 0.0638 (
> 57.7%)
> >>>>>>> Debug Info Emission
> >>>>>>>
> >>>>>>> Before r210492
> >>>>>>> ---User Time--- --System Time-- --User+System-- ---Wall
> Time---
> >>>>>>> --- Name ---
> >>>>>>> 0.0341 ( 62.6%) 0.0261 ( 50.6%) 0.0602 ( 56.8%) 0.0600 (
> 56.7%)
> >>>>>>> Debug Info Emission
> >>>>>>> 0.0350 ( 62.7%) 0.0262 ( 50.4%) 0.0612 ( 56.7%) 0.0612 (
> 56.7%)
> >>>>>>> Debug Info Emission
> >>>>>>> 0.0347 ( 62.7%) 0.0259 ( 50.4%) 0.0607 ( 56.8%) 0.0610 (
> 56.9%)
> >>>>>>> Debug Info Emission
> >>>>>>> 0.0361 ( 62.3%) 0.0267 ( 50.6%) 0.0628 ( 56.7%) 0.0630 (
> 56.7%)
> >>>>>>> Debug Info Emission
> >>>>>>> 0.0354 ( 62.5%) 0.0271 ( 50.4%) 0.0624 ( 56.6%) 0.0625 (
> 56.6%)
> >>>>>>> Debug Info Emission
> >>>>>>>
> >>>>>>> The patch passes check-all. It doesn’t contain any test case as
> it’s
> >>>>>>> just a functionally equivalent refactoring of the same algorithm.
> >>>>>>>
> >>>>>>> Ok to check-in ?
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Fred
> >>>>>>>
> >>>>>>>
> >>>>>>> _______________________________________________
> >>>>>>> llvm-commits mailing list
> >>>>>>> llvm-commits at cs.uiuc.edu
> >>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>>>>>>
> >>>>>
> >>>>
> >>>>
> >>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
--
Alexey Samsonov
vonosmas at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140806/d2602df2/attachment.html>
More information about the llvm-commits
mailing list