[PATCH] D23910: Improve documentation and testing of APIntFromVal
Michael Kruse via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 26 03:06:49 PDT 2016
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
More information about the llvm-commits
mailing list