This is an archive of the discontinued LLVM Phabricator instance.

[APFloat] Fix rotl/rotr when the shift amount is greater than the total bit width.
ClosedPublic

Authored by joey on Dec 14 2016, 3:33 AM.

Details

Reviewers
joey
efriedma
Summary

As per the title, fix the rotation by not truncating the shift amount to the bit width.

Add a unit test to check that 1 << 33 produces 2.

Diff Detail

Event Timeline

joey updated this revision to Diff 81355.Dec 14 2016, 3:33 AM
joey retitled this revision from to [APFloat] Fix rotl/rotr when the shift amount is greater than the total bit width..
joey updated this object.
joey set the repository for this revision to rL LLVM.
joey added a subscriber: llvm-commits.
efriedma requested changes to this revision.Dec 14 2016, 10:56 AM
efriedma added a reviewer: efriedma.
efriedma added a subscriber: efriedma.

There are two problems with the rotl() proposed by this patch:

  1. getZExtValue() will cause an assertion failure if the shift amount is greater than 2^64.
  2. Truncation doesn't work correctly if the bitwidth of the APInt isn't a power of two; you need to use urem or something like that.
This revision now requires changes to proceed.Dec 14 2016, 10:56 AM
joey updated this revision to Diff 83216.Jan 5 2017, 4:46 AM
joey edited edge metadata.

Updated the code to use 'urem' to get the remainder, rather than calling getZExtValue.

Should the APInt rotateAmt need to have the same bitwidth as the APInt the rotl is being called on? urem and a few other methods need that.

Should the APInt rotateAmt need to have the same bitwidth as the APInt the rotl is being called on?

I don't see any reason to add that restriction. (Please add a test for this.)


Please add a testcase for a very large rotate amount (greater than 2^64). Please add testcases for rotating an integer whose bit-width is not a power of two.

lib/Support/APInt.cpp
1249

"APInt(rotBitWidth, BitWidth)" could be zero. (For example, APInt(32, 1).rotl(APInt(1, 1)). Please add this as a testcase.)

joey updated this revision to Diff 83358.Jan 6 2017, 4:57 AM
joey edited edge metadata.
joey removed rL LLVM as the repository for this revision.

Added a test case using the 'Big' number in the unit test.
Added a test case for a 7-bit number.
Added cases where the previous code would assert due to RHS being 0.

joey added inline comments.Jan 6 2017, 5:00 AM
lib/Support/APInt.cpp
1249

I've changed this to declare 'rem' where it's first assigned (line 1253). I'll upload again once Eli has taken another look.

efriedma added inline comments.Jan 6 2017, 10:07 AM
lib/Support/APInt.cpp
1253

Please factor out the common code from rotl and rotr into a helper function. Maybe also add a comment describing why it's implemented in this particular way, since it's a bit subtle.

joey updated this revision to Diff 83604.Jan 9 2017, 3:24 AM
joey set the repository for this revision to rL LLVM.
joey marked 2 inline comments as done.

Created a helper function with some comments.

efriedma added inline comments.Jan 9 2017, 10:39 AM
lib/Support/APInt.cpp
1273

Please move the call to getLimitedValue() into the helper.

joey updated this revision to Diff 85884.Jan 26 2017, 4:34 AM

Moved the getLimitedValue call into the helper function.

Sorry for the delay.

This revision is now accepted and ready to land.Jan 26 2017, 10:34 AM
joey accepted this revision.Feb 7 2017, 4:11 AM

Do you need me to commit this for you?

joey closed this revision.Feb 8 2017, 7:12 AM