[llvm] r228212 - IR: Reduce boilerplate in DenseMapInfo overrides, NFC
Duncan P. N. Exon Smith
dexonsmith at apple.com
Wed Feb 4 16:54:51 PST 2015
> On 2015-Feb-04, at 15:20, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
>>
>> 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?
>
For now I've renamed it to `isKeyOf()` in r228242.
>> (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
>>>
>
>
> _______________________________________________
> 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