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

Thu Aug 18 14:42:42 PDT 2016

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?

>
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160818/f17865d7/attachment.html>