[cfe-commits] [Patch] -Wduplicate-enum which fixes PR6343

[Richard Trieu]

Several changes to this patch.  Richard Smith's suggestions has improved
performance.  Those changes are, only check values 64-bits or smaller and
directly compare the values instead of going through the functions and
using a sorted vector instead of a map to store the values.  Other changes
were to exclude a few more cases, and move the test from SemaCXX to Sema
and then have the RUN line test in both C and C++.

The improved performance has dropped the difference down to under 4%
difference for files purely made of a single enum from a previous ~10%. For
preprocessed Clang files, the difference was under the noise on my system.

On Fri, Jul 27, 2012 at 3:51 PM, Richard Trieu <rtrieu at google.com> wrote:

> Running Clang over itself, using preprocessed files and -fsyntax-only
> 28.86 to 29.08 seconds - Control
> 28.89 to 29.49 seconds - Clang with -Wduplicate-enum
>
> This was run without Richard Smith's suggestions.  I will test those out
> to see if there's any impact from them.
>
>
> On Wed, Jul 25, 2012 at 2:37 PM, Ted Kremenek <kremenek at apple.com> wrote:
>
>> Ok.  That's still a scary number.  Do you have numbers for realistic
>> examples?  For example, we know Clang has some particularly large enums.
>>  This micro benchmark is useful, but it may be overly pessimistic.
>>
>> On Jul 25, 2012, at 2:34 PM, Richard Trieu <rtrieu at google.com> wrote:
>>
>> Yes, that is the slowdown for the entire -fsyntax-only time for a source
>> file with only an enum in it.
>>
>> On Wed, Jul 25, 2012 at 1:48 PM, Ted Kremenek <kremenek at apple.com> wrote:
>>
>>> Hi Richard,
>>>
>>> If I am reading that right, the 6-10% slowdown is for the entire
>>> -fsyntax-only time?  If so, that's definitely cost prohibitive.
>>>
>>> Ted
>>>
>>> On Jul 19, 2012, at 8:25 PM, Richard Trieu <rtrieu at google.com> wrote:
>>>
>>> On Wed, Jul 18, 2012 at 9:14 PM, Ted Kremenek <kremenek at apple.com>wrote:
>>>
>>>> On Jul 18, 2012, at 6:34 PM, Richard Trieu <rtrieu at google.com> wrote:
>>>>
>>>> A set could work for detecting the values, but both EnumConstantDecls
>>>> are needed for the diagnostic, not just the values.  Possibly a map from
>>>> APSInt->EnumConstantDecl* would work.  But either way, I would be dealing
>>>> with getting APSInts to play nice with each other.
>>>>
>>>>
>>>> That seems reasonable to me.  The primary performance issue I see is
>>>> the quadratic algorithmic complexity.  If the APSInt comparisons are an
>>>> issue, we can see if we can find ways to optimize that further.
>>>>
>>>
>>> I created two more variations on and measured some timings.  Both used a
>>> map, one with a custom compare function and one that extended the APSInt
>>> value before insertion.  The APSInt extension had the better time, so I'll
>>> be giving the number for that one.
>>>
>>> At 10,000 elements, there was a 6-10% slow down.  This amounts to
>>> .01-.03 seconds difference on .13-.27 second runtime.
>>>
>>> At 100,000 elements, 8-12% slow down.  .2-.3 seconds on 1.34 to 2.66
>>> second run time.
>>>
>>> At 1,000,000 elements, 7-14% slow down.  Around 2 second difference for
>>> runs of 13.6 to 26.7 seconds.
>>>
>>> A new patch has been attached which has the APSInt bit extension before
>>> adding to the map.
>>> <duplicate-enum-bit-extension.patch>
>>>
>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120813/b8f6f0d7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: duplicate-enum-vector.patch
Type: application/octet-stream
Size: 7802 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120813/b8f6f0d7/attachment.obj>