[PATCH] Improve performance of calculateDbgValueHistory

[Eric Christopher]

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
>> >>
>>
>
>