[PATCH] Improve performance of calculateDbgValueHistory

Adrian Prantl aprantl at apple.com
Wed Aug 6 11:50:44 PDT 2014 

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

More information about the llvm-commits mailing list