[llvm-commits] metadata.h/cpp review stuff

[Devang Patel]

On Mon, Oct 19, 2009 at 12:06 AM, Chris Lattner <clattner at apple.com> wrote:
> Hi Devang,
>
> I'm just now getting caught up on your work.  Here are some thoughts/
> questions on Metadata.h/cpp:

Thanks!

>
> In Metadata.h:
>
>
> #include "llvm/User.h"
> #include "llvm/Type.h"
> #include "llvm/OperandTraits.h"
> #include "llvm/ADT/FoldingSet.h"
> #include "llvm/ADT/SmallVector.h"
> #include "llvm/ADT/SmallPtrSet.h"
> #include "llvm/ADT/DenseMap.h"
> #include "llvm/ADT/StringMap.h"
> #include "llvm/ADT/ilist_node.h"
> #include "llvm/Support/ErrorHandling.h"
> #include "llvm/Support/ValueHandle.h"
>
> This is including way too much.  Please reduce this as aggressively as
> possible.

Yes, this is on my list.

>
>
> class MDString : public MetadataBase {
> ...
>   explicit MDString(LLVMContext &C, const char *begin, unsigned l)
>
> This ctor should just take a StringRef.
>

ok

>
> ...
>   static MDString *get(LLVMContext &Context, const StringRef &Str);
>
> Please take StringRef's by value, not by const&.
>

ok

>
> class MDNode : public MetadataBase, public FoldingSetNode {
> ...
>   friend class ElementVH;
>   // Use CallbackVH to hold MDNOde elements.
>   struct ElementVH : public CallbackVH {
>     MDNode *Parent;
>     ElementVH(Value *V, MDNode *P) : CallbackVH(V), Parent(P) {}
>     ~ElementVH() {}
>
>     virtual void deleted() {
>       Parent->replaceElement(this->operator Value*(), 0);
>     }
>
>     virtual void allUsesReplacedWith(Value *NV) {
>       Parent->replaceElement(this->operator Value*(), NV);
>     }
>   };
>   // Replace each instance of F from the element list of this node
> with T.
>   void replaceElement(Value *F, Value *T);
>
>   SmallVector<ElementVH, 4> Node;
>
>
> Please move all of this stuff out of line to Metadata.cpp.  If needbe,
> Node should be declared as a 'void*' in the header.  Also, there is no
> reason to use a SmallVector here: the size is fixed on node
> construction.  Just use a pointer to array and new[]/delete[] the
> array.  Keeping a SmallVector in NamedMDNode (but not MDNode) is
> completely fine.

ok

>
> Why does MDNode::dropAllReferences need to exist?  It seems *highly*
> unsafe because it clears 'Node' but doesn't update the FoldingSet.

FoldingSet should be updated properly otherwise it is a bug.

>
>   Value *getElement(unsigned i) const {
>     assert(getNumElements() > i && "Invalid element number!");
>
> This is fine, but please use "i < getNumElements()" because that is
> more idiomatic.  This happens multiple times in the file.

ok

>
> /// WeakMetadataVH - a weak value handle for metadata.
> class WeakMetadataVH : public WeakVH {
> public:
>
> I think this should be removed and the use replaced with TrackingVH??

I am also using this to make sure that NamedMDNode collects only
metadata values.

> NamedMDNode::setParent shouldn't be public.  Does
> NamedMDNode::dropAllReferences need to exist?

It is using MDNodes.

> I don't understand NamedMDNode::Create(const NamedMDNode *NMD, Module
> *M).  Is this a weird copy ctor?  Why does it need to exist?

This is used for transferring NamedMDNode from one module to final
linked module during linking.

>
>   void addElement(MetadataBase *M) {
>     resizeOperands(0);
>     OperandList[NumOperands++] = M;
>     Node.push_back(WeakMetadataVH(M));
>   }
>
> How can this work?  Isn't this an invalid access into OperandList?

It is wrong. I'll fix it.

>
> /// MetadataContext -
> /// MetadataContext handles uniquing and assignment of IDs for custom
> metadata
> /// types. Custom metadata handler names do not contain spaces. And
> the name
> /// must start with an alphabet. The regular expression used to check
> name
>
> "start with an alphabet" is missing something.
>
> class MetadataContext {
> public:
>   typedef std::pair<unsigned, WeakVH> MDPairTy;
>   typedef SmallVector<MDPairTy, 2> MDMapTy;
>   typedef DenseMap<const Instruction *, MDMapTy> MDStoreTy;
>   friend class BitcodeReader;
> private:
>
>   /// MetadataStore - Collection of metadata used in this context.
>   MDStoreTy MetadataStore;
>
>   /// MDHandlerNames - Map to hold metadata handler names.
>   StringMap<unsigned> MDHandlerNames;
>
>
> These datastructures are all implementation details.  They should be
> hidden behind a void*, allowing you to avoid #include of DenseMap.h,
> StringMap.h etc.  MDPairTy should be replaced with TrackingVH<MDNode>.
>

ok

>
>   /// RegisterMDKind - Register a new metadata kind and return its ID.
>   /// A metadata kind can be registered only once.
>   unsigned RegisterMDKind(const char *Name);
>
>   /// getMDKind - Return metadata kind. If the requested metadata kind
>   /// is not registered then return 0.
>   unsigned getMDKind(const char *Name);
>
>   /// validName - Return true if Name is a valid custom metadata
> handler name.
>   bool validName(const char *Name);
>
> These methods should all take StringRef for consistency.  The second
> should be const, the third should be static.  validName ->
> isValidName.  RegisterMDKind -> registerMDKind.

ok

>
>   /// getMD - Get the metadata of given kind attached with an
> Instruction.
>   /// If the metadata is not found then return 0.
>   MDNode *getMD(unsigned Kind, const Instruction *Inst);
>
> "with an instruction" -> "to an instruction".   This occurs several
> times in both files.
>

ok

>   /// removeMDs - Remove all metadata attached with an instruction.
>   void removeMDs(const Instruction *Inst);
>
> removeAllMetadata()?

ok :)

>
>
>   /// getHandlerNames - Get handler names. This is used by bitcode
>   /// writer.
>   const StringMap<unsigned> *getHandlerNames();
>
> This is gross for several reasons: at the least, it should be const.
> It also exposes implementation details.  Finally, the bitcode writer
> is currently writing these out in iterator order which is not at all
> deterministic.  It would be better to have something like:
>
>   void getHandlerNames(SmallVectorImpl<std::pair<unsigned, StringRef>
>  > &) const;
>
> and have the implementation of getHandlerNames do the sort (with
> array_pod_sort).

ok

[I'll reply to rest of the comments in separate email :) ]
Thanks for the review!
-
Devang