[llvm] r228212 - IR: Reduce boilerplate in DenseMapInfo overrides, NFC

Duncan P. N. Exon Smith dexonsmith at apple.com
Wed Feb 4 15:20:14 PST 2015


> On 2015-Feb-04, at 15:01, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Wed, Feb 4, 2015 at 2:58 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > On 2015-Feb-04, at 14:54, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Wed, Feb 4, 2015 at 2:08 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> > Author: dexonsmith
> > Date: Wed Feb  4 16:08:30 2015
> > New Revision: 228212
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=228212&view=rev
> > Log:
> > IR: Reduce boilerplate in DenseMapInfo overrides, NFC
> >
> > Minimize the boilerplate required for the `MDNode` subclass
> > `DenseMapInfo<>` overrides in `LLVMContextImpl`.
> >
> > Modified:
> >     llvm/trunk/lib/IR/LLVMContextImpl.h
> >
> > Modified: llvm/trunk/lib/IR/LLVMContextImpl.h
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/LLVMContextImpl.h?rev=228212&r1=228211&r2=228212&view=diff
> > ==============================================================================
> > --- llvm/trunk/lib/IR/LLVMContextImpl.h (original)
> > +++ llvm/trunk/lib/IR/LLVMContextImpl.h Wed Feb  4 16:08:30 2015
> > @@ -179,7 +179,7 @@ protected:
> >        : RawOps(Ops), Hash(calculateHash(Ops)) {}
> >
> >    template <class NodeTy>
> > -  MDNodeOpsKey(NodeTy *N, unsigned Offset = 0)
> > +  MDNodeOpsKey(const NodeTy *N, unsigned Offset = 0)
> >        : Ops(N->op_begin() + Offset, N->op_end()), Hash(N->getHash()) {}
> >
> >    template <class NodeTy>
> > @@ -208,125 +208,97 @@ public:
> >    unsigned getHash() const { return Hash; }
> >  };
> >
> > +template <class NodeTy> struct MDNodeKeyImpl;
> > +template <class NodeTy> struct MDNodeInfo;
> > +
> >  /// \brief DenseMapInfo for MDTuple.
> >  ///
> >  /// Note that we don't need the is-function-local bit, since that's implicit in
> >  /// the operands.
> > -struct MDTupleInfo {
> > -  struct KeyTy : MDNodeOpsKey {
> > -    KeyTy(ArrayRef<Metadata *> Ops) : MDNodeOpsKey(Ops) {}
> > -    KeyTy(MDTuple *N) : MDNodeOpsKey(N) {}
> > -
> > -    bool operator==(const MDTuple *RHS) const {
> > -      if (RHS == getEmptyKey() || RHS == getTombstoneKey())
> > -        return false;
> > -      return compareOps(RHS);
> > -    }
> > -
> > -    static unsigned calculateHash(MDTuple *N) {
> > -      return MDNodeOpsKey::calculateHash(N);
> > -    }
> > -  };
> > -  static inline MDTuple *getEmptyKey() {
> > -    return DenseMapInfo<MDTuple *>::getEmptyKey();
> > -  }
> > -  static inline MDTuple *getTombstoneKey() {
> > -    return DenseMapInfo<MDTuple *>::getTombstoneKey();
> > -  }
> > -  static unsigned getHashValue(const KeyTy &Key) { return Key.getHash(); }
> > -  static unsigned getHashValue(const MDTuple *U) { return U->getHash(); }
> > -  static bool isEqual(const KeyTy &LHS, const MDTuple *RHS) {
> > -    return LHS == RHS;
> > -  }
> > -  static bool isEqual(const MDTuple *LHS, const MDTuple *RHS) {
> > -    return LHS == RHS;
> > +template <> struct MDNodeKeyImpl<MDTuple> : MDNodeOpsKey {
> > +  MDNodeKeyImpl(ArrayRef<Metadata *> Ops) : MDNodeOpsKey(Ops) {}
> > +  MDNodeKeyImpl(const MDTuple *N) : MDNodeOpsKey(N) {}
> > +
> > +  bool operator==(const MDTuple *RHS) const { return compareOps(RHS); }
> >
> > The assymetry here is a bit odd. Probably nicer to give it a name than call it operator== when it's not symmetric (compares distinct types, doesn't work if you compare them in the opposite order, etc - or should the RHS just be MDNodeKeyImpl and rely on implicit conversion through the converting ctor (still better as a (possibly friended) non-member to make RHS conversions and LHS conversions equal/possible)
> 
> Any ideas for a function name?  I feel like `equals()` has the same
> problem.
> 
> Maybe `matches()`?
> 
> Sounds plausible. Worth avoiding the:
> 
> friend bool operator==(const MDNodeKeyImpl &A, const MDNodeKeyImpl &B) { A.compareOps(B); } ?
> 

This operator/function should *only* be used by `MDNodeInfo::isEqual()`
(which is where I got the `operator==()` name from in the first place):

    static bool isEqual(const KeyTy &LHS, const NodeTy *RHS) {
      if (RHS == getEmptyKey() || RHS == getTombstoneKey())
        return false;
      return LHS == RHS;
    }

The keys themselves are never compared with each other.

I'm kind of ambivalent here (could go either way), but given the known
single client, I don't see the benefit.  The cost is a bunch of extra
copies (particularly for some nodes to come that have a lot of fields).
The compiler should be able to optimize those out, but...

Given the limited scope of these `Key` types, what do you think?

> (or is there any reason not to just use/call compareOps directly? But I'm not sure where that function comes from/haven't stared at the DenseMap Key info details long enough to understand how they work)

The other `MDNodeKeyImpl<>`s do more than `compareOps()`.  See the
specializations for `MDLocation` and `GenericDebugNode` (or the patch
I just posted with "IR: Add specialized debug info metadata nodes").

> 
> > +
> > +  unsigned getHashValue() const { return getHash(); }
> > +
> > +  static unsigned calculateHash(MDTuple *N) {
> > +    return MDNodeOpsKey::calculateHash(N);
> >    }
> >  };
> >
> >  /// \brief DenseMapInfo for MDLocation.
> > -struct MDLocationInfo {
> > -  struct KeyTy {
> > -    unsigned Line;
> > -    unsigned Column;
> > -    Metadata *Scope;
> > -    Metadata *InlinedAt;
> > -
> > -    KeyTy(unsigned Line, unsigned Column, Metadata *Scope, Metadata *InlinedAt)
> > -        : Line(Line), Column(Column), Scope(Scope), InlinedAt(InlinedAt) {}
> > -
> > -    KeyTy(const MDLocation *L)
> > -        : Line(L->getLine()), Column(L->getColumn()), Scope(L->getScope()),
> > -          InlinedAt(L->getInlinedAt()) {}
> > -
> > -    bool operator==(const MDLocation *RHS) const {
> > -      if (RHS == getEmptyKey() || RHS == getTombstoneKey())
> > -        return false;
> > -      return Line == RHS->getLine() && Column == RHS->getColumn() &&
> > -             Scope == RHS->getScope() && InlinedAt == RHS->getInlinedAt();
> > -    }
> > -  };
> > -  static inline MDLocation *getEmptyKey() {
> > -    return DenseMapInfo<MDLocation *>::getEmptyKey();
> > -  }
> > -  static inline MDLocation *getTombstoneKey() {
> > -    return DenseMapInfo<MDLocation *>::getTombstoneKey();
> > -  }
> > -  static unsigned getHashValue(const KeyTy &Key) {
> > -    return hash_combine(Key.Line, Key.Column, Key.Scope, Key.InlinedAt);
> > -  }
> > -  static unsigned getHashValue(const MDLocation *U) {
> > -    return getHashValue(KeyTy(U));
> > +template <> struct MDNodeKeyImpl<MDLocation> {
> > +  unsigned Line;
> > +  unsigned Column;
> > +  Metadata *Scope;
> > +  Metadata *InlinedAt;
> > +
> > +  MDNodeKeyImpl(unsigned Line, unsigned Column, Metadata *Scope,
> > +                Metadata *InlinedAt)
> > +      : Line(Line), Column(Column), Scope(Scope), InlinedAt(InlinedAt) {}
> > +
> > +  MDNodeKeyImpl(const MDLocation *L)
> > +      : Line(L->getLine()), Column(L->getColumn()), Scope(L->getScope()),
> > +        InlinedAt(L->getInlinedAt()) {}
> > +
> > +  bool operator==(const MDLocation *RHS) const {
> > +    return Line == RHS->getLine() && Column == RHS->getColumn() &&
> > +           Scope == RHS->getScope() && InlinedAt == RHS->getInlinedAt();
> >    }
> > -  static bool isEqual(const KeyTy &LHS, const MDLocation *RHS) {
> > -    return LHS == RHS;
> > -  }
> > -  static bool isEqual(const MDLocation *LHS, const MDLocation *RHS) {
> > -    return LHS == RHS;
> > +  unsigned getHashValue() const {
> > +    return hash_combine(Line, Column, Scope, InlinedAt);
> >    }
> >  };
> >
> >  /// \brief DenseMapInfo for GenericDebugNode.
> > -struct GenericDebugNodeInfo {
> > -  struct KeyTy : MDNodeOpsKey {
> > -    unsigned Tag;
> > -    StringRef Header;
> > -    KeyTy(unsigned Tag, StringRef Header, ArrayRef<Metadata *> DwarfOps)
> > -        : MDNodeOpsKey(DwarfOps), Tag(Tag), Header(Header) {}
> > -    KeyTy(GenericDebugNode *N)
> > -        : MDNodeOpsKey(N, 1), Tag(N->getTag()), Header(N->getHeader()) {}
> > -
> > -    bool operator==(const GenericDebugNode *RHS) const {
> > -      if (RHS == getEmptyKey() || RHS == getTombstoneKey())
> > -        return false;
> > -      return Tag == RHS->getTag() && Header == RHS->getHeader() &&
> > -             compareOps(RHS, 1);
> > -    }
> > -
> > -    static unsigned calculateHash(GenericDebugNode *N) {
> > -      return MDNodeOpsKey::calculateHash(N, 1);
> > -    }
> > -  };
> > -  static inline GenericDebugNode *getEmptyKey() {
> > -    return DenseMapInfo<GenericDebugNode *>::getEmptyKey();
> > -  }
> > -  static inline GenericDebugNode *getTombstoneKey() {
> > -    return DenseMapInfo<GenericDebugNode *>::getTombstoneKey();
> > +template <> struct MDNodeKeyImpl<GenericDebugNode> : MDNodeOpsKey {
> > +  unsigned Tag;
> > +  StringRef Header;
> > +  MDNodeKeyImpl(unsigned Tag, StringRef Header, ArrayRef<Metadata *> DwarfOps)
> > +      : MDNodeOpsKey(DwarfOps), Tag(Tag), Header(Header) {}
> > +  MDNodeKeyImpl(const GenericDebugNode *N)
> > +      : MDNodeOpsKey(N, 1), Tag(N->getTag()), Header(N->getHeader()) {}
> > +
> > +  bool operator==(const GenericDebugNode *RHS) const {
> > +    return Tag == RHS->getTag() && Header == RHS->getHeader() &&
> > +           compareOps(RHS, 1);
> >    }
> > -  static unsigned getHashValue(const KeyTy &Key) {
> > -    return hash_combine(Key.getHash(), Key.Tag, Key.Header);
> > +
> > +  unsigned getHashValue() const { return hash_combine(getHash(), Tag, Header); }
> > +
> > +  static unsigned calculateHash(GenericDebugNode *N) {
> > +    return MDNodeOpsKey::calculateHash(N, 1);
> >    }
> > -  static unsigned getHashValue(const GenericDebugNode *U) {
> > -    return hash_combine(U->getHash(), U->getTag(), U->getHeader());
> > +};
> > +
> > +/// \brief DenseMapInfo for MDNode subclasses.
> > +template <class NodeTy> struct MDNodeInfo {
> > +  typedef MDNodeKeyImpl<NodeTy> KeyTy;
> > +  static inline NodeTy *getEmptyKey() {
> > +    return DenseMapInfo<NodeTy *>::getEmptyKey();
> > +  }
> > +  static inline NodeTy *getTombstoneKey() {
> > +    return DenseMapInfo<NodeTy *>::getTombstoneKey();
> > +  }
> > +  static unsigned getHashValue(const KeyTy &Key) { return Key.getHashValue(); }
> > +  static unsigned getHashValue(const NodeTy *N) {
> > +    return KeyTy(N).getHashValue();
> >    }
> > -  static bool isEqual(const KeyTy &LHS, const GenericDebugNode *RHS) {
> > +  static bool isEqual(const KeyTy &LHS, const NodeTy *RHS) {
> > +    if (RHS == getEmptyKey() || RHS == getTombstoneKey())
> > +      return false;
> >      return LHS == RHS;
> >    }
> > -  static bool isEqual(const GenericDebugNode *LHS,
> > -                      const GenericDebugNode *RHS) {
> > +  static bool isEqual(const NodeTy *LHS, const NodeTy *RHS) {
> >      return LHS == RHS;
> >    }
> >  };
> >
> > +#define HANDLE_MDNODE_LEAF(CLASS) typedef MDNodeInfo<CLASS> CLASS##Info;
> > +#include "llvm/IR/Metadata.def"
> > +
> >  class LLVMContextImpl {
> >  public:
> >    /// OwnedModules - The set of modules instantiated in this context, and which
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >





More information about the llvm-commits mailing list