[PATCH] Improve performance of calculateDbgValueHistory

David Blaikie dblaikie at gmail.com
Wed Aug 6 10:02:22 PDT 2014 

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

More information about the llvm-commits mailing list