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

Thu Aug 18 15:28:50 PDT 2016

> On 2016-Aug-18, at 14:44, Pete Cooper <peter_cooper at apple.com> wrote:
> 
>> 
>> On Aug 18, 2016, at 2:42 PM, David Majnemer <david.majnemer at gmail.com> wrote:
>> 
>> 
>> 
>> On Thu, Aug 18, 2016 at 1:05 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>> +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?
>> 
>> After talking to some folks on IRC, I agree.  I will try to write a new patch which will introduce GenericAPInt and GenericConstantRange.  Both will be class templates.  APInt and ConstantRange will be typedefs of GenericAPInt<64> and GenericConstantRange<64>.  Major props to Sanjoy for coming up with these.
>> 
>> Sound reasonable?
> Yep, sounds good to me.  I’d be happy to rerun my testing to get the numbers once you have a patch.

SGTM too!

> 
> Cheers,
> Pete
>> 
>> 
>>  
>> 
>> 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