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