[PATCH] D23636: [ADT] Allocate memory less often by increase inline storage

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

A few quick comments.


================
Comment at: include/llvm/ADT/APInt.h:83
@@ -82,3 +82,3 @@
   union {
-    uint64_t VAL;   ///< Used to store the <= 64 bits integer value.
-    uint64_t *pVal; ///< Used to store the >64 bits integer value.
+    uint64_t VAL[2]; ///< Used to store the <= 128 bits integer value.
+    uint64_t *pVal;  ///< Used to store the >128 bits integer value.
----------------
If `VAL` was a pair, or a `struct { uint64_t Low, High; }`, it'd be copyable directly. Also I'd rather read `Val.Low`  instead of `Val[0]`.

================
Comment at: include/llvm/ADT/APInt.h:271
@@ +270,3 @@
+      VAL[0] = val;
+    } else if (isTwoWords()) {
+      VAL[0] = val;
----------------
Spurious braces for the if?

================
Comment at: include/llvm/ADT/APInt.h:273
@@ +272,3 @@
+      VAL[0] = val;
+      VAL[1] = isSigned && int64_t(val) < 0 ? -1ULL : 0;
+    } else {
----------------
I'd have missed this one :)

================
Comment at: include/llvm/ADT/APInt.h:276
@@ -246,2 +275,3 @@
       initSlowCase(val, isSigned);
+    }
     clearUnusedBits();
----------------
Remove braces?

================
Comment at: include/llvm/ADT/APInt.h:322
@@ -287,2 +321,3 @@
       initSlowCase(that);
+    }
   }
----------------
Same braces as above.

================
Comment at: include/llvm/ADT/APInt.h:329
@@ -292,1 +328,3 @@
+    if (isTwoWords())
+      VAL[1] = that.VAL[1];
     that.BitWidth = 0;
----------------
Couldn't you just copy VAL as whole all the time?

================
Comment at: include/llvm/ADT/APInt.h:344
@@ -305,3 +343,3 @@
   ///  method Read).
-  explicit APInt() : BitWidth(1), VAL(0) {}
+  explicit APInt() : BitWidth(1) { VAL[0] = 0; }
 
----------------
I guess the fact that VAL[1] can be initialized makes explains why you're copying only part of it. Do you expect better code with this? I feel it is easier to think about VAL as a single unit as much as possible.

================
Comment at: include/llvm/ADT/APInt.h:436
@@ -394,1 +435,3 @@
+    if (isTwoWords())
+      return VAL[0] != VAL[1] && isPowerOf2_64(VAL[0] | VAL[1]);
     return countPopulationSlowCase() == 1;
----------------
`VAL[0] == 0 && isPowerOf2_64(VAL[1])` ?

================
Comment at: include/llvm/ADT/APInt.h:720
@@ -672,3 +719,3 @@
     // as modified.
-    memcpy(&VAL, &that.VAL, sizeof(uint64_t));
+    memcpy(&VAL, &that.VAL, sizeof(uint64_t) * 2);
 
----------------
Any risk that you're reading uninitialized memory here? (in line with comment in the default ctor above)


https://reviews.llvm.org/D23636