[llvm-commits] [PATCH] Rotate constant folding bug

[Cameron McInally]

Hi Eli,

Please find the updated patch and simple unit test attached. I would have
liked to add a more thorough unit test, but I believe there's a bug in the
unit tests also. Could you please confirm the following...

Looking at TEST(APIntTest, i1) in unittests/ADT/APIntTest.cpp, I see an
APInt defined as:

> const APInt two(1, 2);

If I'm not mistaken, that's one bit representing the unsigned value '2'. So,
effectively, that APInt is a '0'. Unless this was done deliberately (which
is why I'm asking), I believe the unit tests following this definition are
not doing what they are supposed to do.

If you can confirm, I can take a whack at straightening things out.

Thanks again,
Cameron

On Tue, Oct 11, 2011 at 10:09 PM, Eli Friedman <eli.friedman at gmail.com>wrote:

> On Tue, Oct 11, 2011 at 6:06 PM, Cameron McInally
> <cameron.mcinally at nyu.edu> wrote:
> > Hey guys,
> >
> > Constant folding for the rotate operators does not work currently. Here's
> a
> > fix...
> >
> > royale mcinally/llvm> svn diff
> >
> > Index: lib/Support/APInt.cpp
> >
> > ===================================================================
> >
> > --- lib/Support/APInt.cpp (revision 141748)
> >
> > +++ lib/Support/APInt.cpp (working copy)
> >
> > @@ -1334,8 +1334,8 @@
> >
> >    // Don't get too fancy, just use existing shift/or facilities
> >
> >    APInt hi(*this);
> >
> >    APInt lo(*this);
> >
> > -  hi.shl(rotateAmt);
> >
> > -  lo.lshr(BitWidth - rotateAmt);
> >
> > +  hi = hi.shl(rotateAmt);
> >
> > +  lo = lo.lshr(BitWidth - rotateAmt);
> >
> >    return hi | lo;
> >
> >  }
> >
> >
> >
> > @@ -1349,8 +1349,8 @@
> >
> >    // Don't get too fancy, just use existing shift/or facilities
> >
> >    APInt hi(*this);
> >
> >    APInt lo(*this);
> >
> > -  lo.lshr(rotateAmt);
> >
> > -  hi.shl(BitWidth - rotateAmt);
> >
> > +  lo = lo.lshr(rotateAmt);
> >
> > +  hi = hi.shl(BitWidth - rotateAmt);
> >
> >    return hi | lo;
> >
> >  }
>
> Err, wow.  Nice catch.  Would you mind adding a test in
> unittests/ADT/APIntTest.cpp ?    (Also, we prefer patches to be
> attached rather than inlined.)
>
> -Eli
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111013/53658d31/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rotate_patch
Type: application/octet-stream
Size: 1384 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111013/53658d31/attachment.obj>