[llvm] r233881 - Fix a bug indicated by -fsanitize=shift-exponent.
Alexey Samsonov
vonosmas at gmail.com
Fri Apr 3 17:58:43 PDT 2015
+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>
More information about the llvm-commits
mailing list