[PATCH] Improve performance of calculateDbgValueHistory

Adrian Prantl aprantl at apple.com
Wed Aug 6 09:35:35 PDT 2014 

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.

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

More information about the llvm-commits mailing list