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

[Alexey Samsonov]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150403/87e46ff9/attachment.html>