Improve documentation and testing for isl_valFromAPInt

[grosser]

Summary

The recent unit tests we gained made clear that the semantics of
isl_valFromAPInt are not clear, due to missing documentation. In this change we
document both the calling interface as well as the implementation of
isl_valFromAPInt. We also make the implementation easier to read by removing
integer wrappig in abs() when passing in the minimal integer value for a given
bitwidth. Even though wrapping and subsequently interpreting the result as
unsigned value gives the correct result, this is far from obvious. Instead, we
explicitly add one more bit to the input type to ensure that abs will never
wrap. Finally, we add a test case for this special case and also test a number
larger than 64 bit.

[Meinersbur]

LGTM

lib/Support/GICHelper.cpp

45 ↗

(On Diff #69341)

Is this a bug discovered by the additional tests?

unittests/Isl/IslTest.cpp

117 ↗

(On Diff #69341)

I reversed the arguments of EXPECT_EQ in APIntToIslVal because how a fail is printed:

C:\Users\Meinersbur\src\llvm\tools\polly\unittests\Isl\IslTest.cpp(71): error: Value of: 1
Expected: APNOne
Which is: 16-byte object <02-00 00-00 F6-7F 00-00 02-00 00-00 00-00 00-00>

should we reverse all of them (for consistency),such that the output would be:

C:\Users\Meinersbur\src\llvm\tools\polly\unittests\Isl\IslTest.cpp(71): error: Value of: APNOne
  Actual: 16-byte object <02-00 00-00 F6-7F 00-00 02-00 00-00 00-00 00-00>
Expected: 1

[grosser]

Thank you for your comments. I explained in the commit message that the implementation change did not uncover any correctness issues (as I am aware of) and also reordered the arguments of EXPT_EQ in APIntToIslVal. In a subsequent commit I will do the same for APIntFromVal.

[Meinersbur]

In D23917#526087, @grosser wrote:

Thank you for your comments. I explained in the commit message that the implementation change did not uncover any correctness issues (as I am aware of) and also reordered the arguments of EXPT_EQ in APIntToIslVal.

I missed the summary message, sorry. I should give it more attention in reviews.

polly/trunk/include/polly/Support/GICHelper.h

 struct isl_val;
 struct isl_space;
 
 namespace llvm {
 class Value;
 } // namespace llvm
 
 namespace polly {
+
+/// @brief Translate an llvm::APInt to an isl_val.
+///
+/// Translate the bitsequence without sign information as provided by APInt into
+/// a signed isl_val type. Depending on the value of @p IsSigned @p Int is
+/// interpreted as unsigned value or as signed value in two's complement
+/// representation.
+///
+/// Input IsSigned                 Output
+///
+///     0        0           ->    0
+///     1        0           ->    1
+///    00        0           ->    0
+///    01        0           ->    1
+///    10        0           ->    2
+///    11        0           ->    3
+///
+///     0        1           ->    0
+///     1        1           ->   -1
+///    00        1           ->    0
+///    01        1           ->    1
+///    10        1           ->   -2
+///    11        1           ->   -1
+///
+/// @param Ctx      The isl_ctx to create the isl_val in.
+/// @param Int      The integer value to translate.
+/// @param IsSigned If the APInt should be interpreted as signed or unsigned
+///                 value.
+///
+/// @return The isl_val corresponding to @p Int.
 __isl_give isl_val *isl_valFromAPInt(isl_ctx *Ctx, const llvm::APInt Int,
                                      bool IsSigned);
 
 /// @brief Translate isl_val to llvm::APInt.
 ///
 /// This function can only be called on isl_val values which are integers.
 /// Calling this function with a non-integral rational, NaN or infinity value
 /// is not allowed.

polly/trunk/lib/Support/GICHelper.cpp

 
 using namespace llvm;
 
 __isl_give isl_val *polly::isl_valFromAPInt(isl_ctx *Ctx, const APInt Int,
                                             bool IsSigned) {
   APInt Abs;
   isl_val *v;
 
+  // As isl is interpreting the input always as unsigned value, we need some
+  // additional pre and post processing to import signed values. The approach
+  // we take is to first obtain the absolute value of Int and then negate the
+  // value after it has been imported to isl.
+  //
+  // It should be noted that the smallest integer value represented in two's
+  // complement with a certain amount of bits does not have a corresponding
+  // positive representation in two's complement representation with the same
+  // number of bits. E.g. 110 (-2) does not have a corresponding value for (2).
+  // To ensure that there is always a corresponding value available we first
+  // sign-extend the input by one bit and only then take the absolute value.
   if (IsSigned)
-    Abs = Int.abs();
+    Abs = Int.sext(Int.getBitWidth() + 1).abs();
   else
     Abs = Int;
 
   const uint64_t *Data = Abs.getRawData();
   unsigned Words = Abs.getNumWords();
 
   v = isl_val_int_from_chunks(Ctx, Words, sizeof(uint64_t), Data);
 

polly/trunk/unittests/Isl/IslTest.cpp

 using namespace polly;
 
 namespace {
 
 TEST(Isl, APIntToIslVal) {
   isl_ctx *IslCtx = isl_ctx_alloc();
 
   {
+    APInt APZero(1, 0, true);
+    auto *IslZero = isl_valFromAPInt(IslCtx, APZero, true);
+    EXPECT_EQ(isl_bool_true, isl_val_is_zero(IslZero));
+    isl_val_free(IslZero);
+  }
+
+  {
+    APInt APNOne(1, -1, true);
+    auto *IslNOne = isl_valFromAPInt(IslCtx, APNOne, true);
+    EXPECT_EQ(isl_bool_true, isl_val_is_negone(IslNOne));
+    isl_val_free(IslNOne);
+  }
+
+  {
+    APInt APZero(1, 0, false);
+    auto *IslZero = isl_valFromAPInt(IslCtx, APZero, false);
+    EXPECT_EQ(isl_bool_true, isl_val_is_zero(IslZero));
+    isl_val_free(IslZero);
+  }
+
+  {
+    APInt APOne(1, 1, false);
+    auto *IslOne = isl_valFromAPInt(IslCtx, APOne, false);
+    EXPECT_EQ(isl_bool_true, isl_val_is_one(IslOne));
+    isl_val_free(IslOne);
+  }
+
+  {
+    APInt APNTwo(2, -2, true);
+    auto *IslNTwo = isl_valFromAPInt(IslCtx, APNTwo, true);
+    auto *IslNTwoCmp = isl_val_int_from_si(IslCtx, -2);
+    EXPECT_EQ(isl_bool_true, isl_val_eq(IslNTwo, IslNTwoCmp));
+    isl_val_free(IslNTwo);
+    isl_val_free(IslNTwoCmp);
+  }
+
+  {
     APInt APNOne(32, -1, true);
     auto *IslNOne = isl_valFromAPInt(IslCtx, APNOne, true);
     EXPECT_EQ(isl_bool_true, isl_val_is_negone(IslNOne));
     isl_val_free(IslNOne);
   }
 
   {
     APInt APZero(32, 0, false);
     auto *IslZero = isl_valFromAPInt(IslCtx, APZero, false);
-    EXPECT_EQ(isl_val_is_zero(IslZero), isl_bool_true);
+    EXPECT_EQ(isl_bool_true, isl_val_is_zero(IslZero));
     isl_val_free(IslZero);
   }
 
   {
     APInt APOne(32, 1, false);
     auto *IslOne = isl_valFromAPInt(IslCtx, APOne, false);
-    EXPECT_EQ(isl_val_is_one(IslOne), isl_bool_true);
+    EXPECT_EQ(isl_bool_true, isl_val_is_one(IslOne));
     isl_val_free(IslOne);
   }
 
   {
     APInt APTwo(32, 2, false);
     auto *IslTwo = isl_valFromAPInt(IslCtx, APTwo, false);
-    EXPECT_EQ(isl_val_cmp_si(IslTwo, 2), 0);
+    EXPECT_EQ(0, isl_val_cmp_si(IslTwo, 2));
     isl_val_free(IslTwo);
   }
 
   {
     APInt APNOne(32, (1ull << 32) - 1, false);
     auto *IslNOne = isl_valFromAPInt(IslCtx, APNOne, false);
     auto *IslRef = isl_val_int_from_ui(IslCtx, (1ull << 32) - 1);
-    EXPECT_EQ(isl_val_eq(IslNOne, IslRef), isl_bool_true);
+    EXPECT_EQ(isl_bool_true, isl_val_eq(IslNOne, IslRef));
     isl_val_free(IslNOne);
     isl_val_free(IslRef);
   }
 
+  {
+    APInt APLarge(130, 2, false);
+    APLarge = APLarge.shl(70);
+    auto *IslLarge = isl_valFromAPInt(IslCtx, APLarge, false);
+    auto *IslRef = isl_val_int_from_ui(IslCtx, 71);
+    IslRef = isl_val_2exp(IslRef);
+    EXPECT_EQ(isl_bool_true, isl_val_eq(IslLarge, IslRef));
+    isl_val_free(IslLarge);
+    isl_val_free(IslRef);
+  }
+
   isl_ctx_free(IslCtx);
 }
 
 TEST(Isl, IslValToAPInt) {
   isl_ctx *IslCtx = isl_ctx_alloc();
 
   {
     auto *IslNOne = isl_val_int_from_si(IslCtx, -1);