[llvm-commits] [PATCH] Make Inits FoldingSetNodes

[David A. Greene]

Chris Lattner <clattner at apple.com> writes:

> What is the motivation for this?  This patch looks mostly ok to me,
> with some specific requests below.  Note that it is generally bad form
> to ask for a patch to be reviewed before the dependent patches are
> also available.

I wanted to get things in the pipeline.  I'm please how quickly reviews
came in.  Thank you!

The motivation for this is to formalize Init creation and destruction.
As it is, we generate a ton of Inits and almost never delete them.  I'm
doing this because the AVX code is going to create a lot more Inits and
a lot of them will look the same.

> A pre-approved good way to get the mechanical part of this patch in is
> to *just* change "new whatever" to "whatever::get" where
> "whatever::get" is defined in terms of new.  That will get the
> mechanical part of the patch out of the way and make it much easier to
> review the actual changes you're making.

I considered that but thought it best to keep the logical change
together.  Noted for the future.

> That said, here's some thoughts:
>
>     @@ -371,8 +372,40 @@ RecTy *llvm::resolveTypes(RecTy *T1, RecTy *T2) {
>     //    Initializer implementations
>     //===----------------------------------------------------------------------===//
>    
>     +FoldingSet<Init> Init::UniqueInits;
>     +BumpPtrAllocator Init::InitAllocator;
>
> Please don't introduce more global variables with static ctors into
> things.  tblgen is not exactly a paragon of cleanliness, but it would
> be nice to not make it any worse.  If there is no good alternative,
> since this is "just tblgen" it is ok.  In that case, just make it a
> static global in the .cpp file instead of exposing it in the header as
> a static class member.

Ok.

>     +
>     void Init::dump() const { return print(errs()); }
>    
>     +const UnsetInit *UnsetInit::get() {
>     +  FoldingSetNodeID ID;
>     +  ID.AddInteger(initUnset);
>
> There is only one of these, why put it into a folding set?

Consistency.  I'm not sure there is only one of these.

>     +const BitInit *BitInit::get(bool V) {
>     +  FoldingSetNodeID ID;
>     +  ID.AddInteger(initBit);
>     +  ID.AddBoolean(V);
>
> Likewise, there are only two of these.

Ditto.

>     +const IntInit *IntInit::get(int64_t V) {
>     +  FoldingSetNodeID ID;
>     +  ID.AddInteger(initInt);
>     +  ID.AddInteger(V);
>
> Why use a FoldingSet for this instead of a DenseMap?  It seems like completely overkill.
>
>     +const StringInit *StringInit::get(const std::string &V) {
>     +  FoldingSetNodeID ID;
>     +  ID.AddInteger(initString);
>     +  ID.AddString(V);
>
> Similarly, should be a stringmap.
>
>     +const CodeInit *CodeInit::get(const std::string &V) {
>     +  FoldingSetNodeID ID;
>     +  ID.AddInteger(initCode);
>     +  ID.AddString(V);
>
> again.

Ok, I'm confused.  I thought FoldingSet existed to implement the
Flyweight pattern.  That's what I'm trying to do here.  It's not enough
to put the integer values in a (static) DenseMap and the string values
in a (static) StringMap.  I want to get rid of the duplicate objects
_around_ the integer and string values.

                                -Dave