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

[David Majnemer via llvm-commits]

majnemer added inline 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.
----------------
mehdi_amini wrote:
> 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]`.
It might be a little misleading given that `High` would be uninitialized under certain circumstances.

================
Comment at: include/llvm/ADT/APInt.h:271
@@ +270,3 @@
+      VAL[0] = val;
+    } else if (isTwoWords()) {
+      VAL[0] = val;
----------------
mehdi_amini wrote:
> Spurious braces for the if?
I think its most common to brace all the `if/else if` if any of them are braced.

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

================
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; }
 
----------------
mehdi_amini wrote:
> 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.
I prefer not initializing it if it will never be used.  This allows tools like valgrind and ubsan find bugs.

================
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;
----------------
mehdi_amini wrote:
> `VAL[0] == 0 && isPowerOf2_64(VAL[1])` ?
I think your formulation is wrong if `VAL[0]` is 1 and `VAL[1]` is 0.

================
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);
 
----------------
mehdi_amini wrote:
> Any risk that you're reading uninitialized memory here? (in line with comment in the default ctor above)
Good point, I'll amend this.


https://reviews.llvm.org/D23636