[llvm] r233881 - Fix a bug indicated by -fsanitize=shift-exponent.

[Alexey Samsonov]

+Sanjoy who probably knows more about this code.

On Fri, Apr 3, 2015 at 5:57 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:

>
> On Fri, Apr 3, 2015 at 5:46 PM, Justin Bogner <mail at justinbogner.com>
> wrote:
>
>> Alexey Samsonov <vonosmas at gmail.com> writes:
>> > Hi Justin,
>> >
>> > On Fri, Apr 3, 2015 at 5:05 PM, Justin Bogner <mail at justinbogner.com>
>> wrote:
>> >
>> >     Alexey Samsonov <vonosmas at gmail.com> writes:
>> >     > Author: samsonov
>> >     > Date: Wed Apr  1 20:30:10 2015
>> >     > New Revision: 233881
>> >     >
>> >     > URL: http://llvm.org/viewvc/llvm-project?rev=233881&view=rev
>> >     > Log:
>> >     > Fix a bug indicated by -fsanitize=shift-exponent.
>> >
>> >     I noticed a significant performance regression
>> Benchmarks/Shootout/sieve
>> >     after this change. I suspect this isn't quite the right fix for the
>> >     undefined behaviour.
>> >
>> >     You can see the performance jump in lnt here:
>> >
>> >
>> http://llvm-lnt.herokuapp.com/db_default/v4/nts/graph?plot.0=3.794.3&
>> >     highlight_run=9976
>> >
>> >     There's a big regression between r233879 and r233882. This is the
>> only
>> >     interesting change in that range.
>> >
>> >     > Modified:
>> >     >     llvm/trunk/lib/Transforms/Utils/LoopUnrollRuntime.cpp
>> >     >
>> >     > Modified: llvm/trunk/lib/Transforms/Utils/LoopUnrollRuntime.cpp
>> >     > URL:
>> >     >
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/
>> >     LoopUnrollRuntime.cpp?rev=233881&r1=233880&r2=233881&view=diff
>> >     >
>> ========================================================================
>> >     ======
>> >     > --- llvm/trunk/lib/Transforms/Utils/LoopUnrollRuntime.cpp
>> (original)
>> >     > +++ llvm/trunk/lib/Transforms/Utils/LoopUnrollRuntime.cpp Wed
>> Apr  1
>> >     20:30:10 2015
>> >     > @@ -320,7 +320,7 @@ bool llvm::UnrollRuntimeLoopProlog(Loop
>> >     >    // This constraint lets us deal with an overflowing trip count
>> >     easily; see the
>> >     >    // comment on ModVal below.  This check is equivalent to
>> `Log2(Count)
>> >     <
>> >     >    // BEWidth`.
>> >     > -  if (static_cast<uint64_t>(Count) > (1ULL << BEWidth))
>> >     > +  if (BEWidth < 64 && static_cast<uint64_t>(Count) > (1ULL <<
>> BEWidth))
>> >
>> >     To match what the comment says we're doing, I guess we want:
>> >
>> >       if (BEWidth >= 64 || static_cast<uint64_t>(Count) > (1ULL <<
>> BEWidth))
>> >
>> >     since Log2(Count) is guaranteed to be less than 64.
>> >
>> > ... but if BEWidth is 64, than Count can never be greater than 1ULL <<
>> > BEWidth, and we should *not* return false.
>> > Note that the comment doesn't match the current code.
>>
>> Oh, the code says greater, where the comment says less. Strange. I
>> certainly meant to use less in my suggested change, but I misread and
>> thought the current code was using less as well.
>>
>> In any case, it seems whatever we ended up doing when we hit the
>> undefined behaviour generated faster code for the benchmark ;)
>>
>
> Heh, if BEWidth is 64 than (1ULL << BEWidth) will overflow and will likely
> be just 1
> (at least that's how gcc and clang behave on my machine), and we will
> break from the
> function and return false, instead of doing the actual unrolling below.
>
>
>>
>> >
>> >     >      return false;
>> >     >
>> >     >    // If this loop is nested, then the loop unroller changes the
>> code in
>> >     >
>> >     >
>> >     > _______________________________________________
>> >     > llvm-commits mailing list
>> >     > llvm-commits at cs.uiuc.edu
>> >     > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>> > --
>> > Alexey Samsonov
>> > vonosmas at gmail.com
>>
>
>
>
> --
> Alexey Samsonov
> vonosmas at gmail.com
>



-- 
Alexey Samsonov
vonosmas at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150403/6143d906/attachment.html>