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

[Pete Cooper via llvm-commits]

+Duncan

Hi David

I had also found issues with APInt (see r276470, r271556, r270959, and especially r271020).

The vast majority I found on my use case (opt -O2 verify-uselistorder-nodbg.lto.bc) were due to ConstantRange, which is ultimately called from SCEV.

I regenerated a trace today and I still see about 8.8m APInt allocations, of which a glance suggests almost all come from ConstantRange.  So I don’t think what you are doing here is going to be a worry for peak memory, as these APInt’s I see are mostly on the stack.

I CCed Duncan because we’d seen APInt issues in the past.  One solution we considered was to have a template on APInt, e.g., APInt<256> would be an APInt with 256 bits of storage.  The default would be 64 to match the current code.  In reply to r 276470, David Blaikie also suggested something which I think is similar to this.

If we had such an APInt<unsigned BitCount> then ConstantRange could have a typedef somewhere for what size of APInt to use.  The rest of the compiler could remain unchanged.  Alternatively we could extract most of the APInt arithmetic logic to a base class and have APInt/SmallAPInt inherit from this.

What this would mean in terms of the implementation is that APInt would need all of the arithmetic logic to do the fast path when isSingleWord() is true, and share the current slow path between the small storage and the malloc’ed storage.  I think most of the arithmetic is already of this form, but we would need to go check all of it.

What do you think?

BTW, if this sounds gross, alternatives are probably to move away from looking at APInt and see what else can be done in ConstantRange itself.  Unless your trace shows that the APInt traffic is elsewhere?  I’m happy to discuss more alternatives if you prefer though.

Cheers,
Pete
> On Aug 17, 2016, at 5:43 PM, Mehdi Amini via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> 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
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits