[PATCH] D23910: Improve documentation and testing of APIntFromVal

[Michael Kruse via llvm-commits]

Meinersbur added a comment.

Thank you for the extensive documentation and adding more test cases.


================
Comment at: include/polly/Support/GICHelper.h:46
@@ +45,3 @@
+/// This function can only be called on isl_val values which are integers and
+/// asserts in case a non-integral rational, NaN or infinity value is given.
+///
----------------
assert() is not a guaranteed behavior. Better mention that the function can only be called on integer `isl_val`s.

================
Comment at: include/polly/Support/GICHelper.h:55-64
@@ +54,12 @@
+///
+///   Input      Bits    Signed  Unsigned
+///       0 ->      0         0         0
+///      -1 ->      1        -1         1
+///       1 ->     01         1         1
+///      -2 ->     10        -2         2
+///       2 ->    010         2         2
+///      -3 ->    101        -3         5
+///       3 ->    011         3         3
+///      -4 ->    100        -4         4
+///       4 ->   0100         4         4
+///
----------------
Cool! More than I hoped for.

I'd remove the Unsigned column. It gives the impression that an unsigned interpretation would make sense. But maybe add a bitwidth column?

================
Comment at: lib/Support/GICHelper.cpp:65
@@ +64,3 @@
+  // number. In case Val was originally negative, we expand the size of A by
+  // one and negate the value. As a result, the new value in A corresponds
+  // now with Val.
----------------
Can you mention 'negate'=='two's complement'?

================
Comment at: unittests/Isl/IslTest.cpp:68
@@ -67,3 +67,3 @@
     auto APNOne = APIntFromVal(IslNOne);
     // APInt has no sign bit, so never equals to a negative number.
     // FIXME: The canonical representation of a negative APInt is two's
----------------
Suggestion:
"Compare with the two's complement of -1 in a 1-bit integer"

================
Comment at: unittests/Isl/IslTest.cpp:69-70
@@ -68,4 +68,4 @@
     // APInt has no sign bit, so never equals to a negative number.
     // FIXME: The canonical representation of a negative APInt is two's
     // complement.
     EXPECT_EQ(APNOne, 1);
----------------
Remove fixme

================
Comment at: unittests/Isl/IslTest.cpp:150
@@ +149,3 @@
+    auto *IslExp = isl_val_int_from_ui(IslCtx, 500);
+    auto *IslLargePow2 = isl_val_2exp(IslExp);
+    auto APLargePow2 = APIntFromVal(IslLargePow2);
----------------
Wow! Truly large integers.


https://reviews.llvm.org/D23910