[llvm-commits] [llvm] r82060 - in /llvm/trunk: include/llvm/LLVMContext.h include/llvm/Metadata.h include/llvm/Value.h lib/VMCore/LLVMContext.cpp lib/VMCore/LLVMContextImpl.h lib/VMCore/Metadata.cpp lib/VMCore/Value.cpp

[Chris Lattner]

On Sep 16, 2009, at 11:09 AM, Devang Patel wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=82060&view=rev
> Log:
> Add llvm::Metadata to manage metadata used in a context.
> This interface will be used to attach metadata with an instruction.

Hi Devang,

Just getting caught up on commits...

> +++ llvm/trunk/include/llvm/Metadata.h Wed Sep 16 13:09:00 2009

+#include "llvm/ADT/SmallSet.h"

This isn't needed, please remove.

> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +/// Metadata -
> +/// Metadata manages metadata used in a context.
> +
> +/// MDKindID - This id identifies metadata kind the metadata store.  
> Valid
> +/// ID values are 1 or higher. This ID is set by RegisterMDKind.
> +typedef unsigned MDKindID;
> +class Metadata {

Please rename this MetadataContext or something like that.  This class  
*isn't* metadata, it is meta-metadata :).

Also, please don't use global typedefs like MDKindID.  If it is  
important enough to give the type a name (other than 'unsigned'),  
please use a wrapper struct so that it is "a type safe typedef".

> +private:
> +  typedef std::pair<MDKindID, WeakVH> MDPairTy;
> +  typedef SmallVector<MDPairTy, 2> MDMapTy;
> +  typedef DenseMap<const Instruction *, MDMapTy> MDStoreTy;
> +
> +  /// MetadataStore - Collection of metadata used in this context.
> +  MDStoreTy MetadataStore;
> +
> +  /// MDHandlerNames - Map to hold metadata handler names.
> +  StringMap<unsigned> MDHandlerNames;

It is unfortunate that implementation details of this class require  
pulling in #includes.  Please change these to be void*'s in the header  
and cast to the right type in the .cpp file.  An example of this is  
llvm/Target/TargetLoweringObjectFile.h with "UniquingMap".   
Alternatively, put all of this stuff into a "MetadataContextImpl"  
class, and make this class have a pointer to that.

> +public:
> +
> +  /// RegisterMDKind - Register a new metadata kind and return its  
> ID.
> +  /// A metadata kind can be registered only once.
> +  MDKindID RegisterMDKind(const char *Name);
> +
> +  /// getMDKind - Return metadata kind. If the requested metadata  
> kind
> +  /// is not registered then return 0.
> +  MDKindID getMDKind(const char *Name);

The doxygen comment for this class should explain that this class is  
the thing that handles uniquing and assignment of IDs for custom  
metadata types, and should explain the restrictions on the names.


> +  /// getMD - Get the metadata of given kind attached with an  
> Instruction.
> +  /// If the metadata is not found then return 0.
> +  MDNode *getMD(MDKindID Kind, const Instruction *Inst);
> +
> +  /// setMD - Attach the metadata of given kind with an Instruction.
> +  void setMD(MDKindID Kind, MDNode *Node, Instruction *Inst);

This should probably be named "AddMD" not "setMD".  "set" implies that  
there can only be one.

There should also be a "remove" method.

> +
> +  /// ValueIsDeleted - This handler is used to update metadata store
> +  /// when a value is deleted.
> +  void ValueIsDeleted(Value *V) {}
> +  void ValueIsDeleted(const Instruction *Inst);

Is there any reason that Value needs to explicitly know about this?   
Why not use the existing CallbackVH interfaces?  If it would be much  
more inefficient or something, then I can see having a custom  
extension like this, but it would be nice to minimize this if possible.

> +++ llvm/trunk/lib/VMCore/Metadata.cpp Wed Sep 16 13:09:00 2009
> @@ -15,6 +15,7 @@
> #include "llvm/Metadata.h"
> #include "llvm/LLVMContext.h"
> #include "llvm/Module.h"
> +#include "llvm/Instruction.h"
> #include "SymbolTableListTraitsImpl.h"
> using namespace llvm;
>
> @@ -251,3 +252,74 @@
> NamedMDNode::~NamedMDNode() {
>   dropAllReferences();
> }
> +
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +//Metadata implementation
> +//
> +
> +/// RegisterMDKind - Register a new metadata kind and return its ID.
> +/// A metadata kind can be registered only once.
> +MDKindID Metadata::RegisterMDKind(const char *Name) {

This needs to enforce various constraints on the name.  For example,  
you can't have a space in the name etc.  Whatever requirements the .ll  
lexer have should be enforced here with an assert.

> +  MDKindID Count = MDHandlerNames.size();
> +  StringMap<unsigned>::iterator I = MDHandlerNames.find(Name);
> +  assert(I == MDHandlerNames.end() && "Already registered MDKind!");

Please only do the lookup in MDHandlerNames.find when assertions are  
enabled.

> +  MDHandlerNames[Name] = Count + 1;
> +  return Count + 1;
> +}
> +
> +/// getMDKind - Return metadata kind. If the requested metadata kind
> +/// is not registered then return 0.
> +MDKindID Metadata::getMDKind(const char *Name) {
> +  StringMap<unsigned>::iterator I = MDHandlerNames.find(Name);
> +  if (I == MDHandlerNames.end())
> +    return 0;
> +
> +  return I->getValue();
> +}
> +
> +/// setMD - Attach the metadata of given kind with an Instruction.
> +void Metadata::setMD(MDKindID MDKind, MDNode *Node, Instruction  
> *Inst) {
> +  MDStoreTy::iterator I = MetadataStore.find(Inst);
> +  Inst->HasMetadata = true;
> +  if (I == MetadataStore.end()) {
> +    MDMapTy Info;
> +    Info.push_back(std::make_pair(MDKind, Node));
> +    MetadataStore.insert(std::make_pair(Inst, Info));
> +    return;
> +  }
> +
> +  MDMapTy &Info = I->second;
> +  Info.push_back(std::make_pair(MDKind, Node));
> +  return;

It is simpler to write this as:

void Metadata::setMD(MDKindID MDKind, MDNode *Node, Instruction *Inst) {
   asert(Node == 0 && "Use removeMD to remove metadata");

   Inst->HasMetadata = true;
   MDMapTy &Entry = MetadataStore[I];
   if (Entry doesn't already have a "MDKind" entry)
     Entry.push_back(std::make_pair(MDKind, Node));
   else // Otherwise replace it.
     Entry[j] = Node;
}

We don't want to allow multiple entries for one MDKind.


> +/// getMD - Get the metadata of given kind attached with an  
> Instruction.
> +/// If the metadata is not found then return 0.
> +MDNode *Metadata::getMD(MDKindID MDKind, const Instruction *Inst) {
> +  MDNode *Node = NULL;
> +  MDStoreTy::iterator I = MetadataStore.find(Inst);
> +  if (I == MetadataStore.end())
> +    return Node;
> +
> +  MDMapTy &Info = I->second;
> +  for (MDMapTy::iterator I = Info.begin(), E = Info.end(); I != E; + 
> +I)
> +    if (I->first == MDKind)
> +      Node = dyn_cast_or_null<MDNode>(I->second);

We shouldn't allow duplicates, so this can be simplified to early  
return when MDKind is found.  You can eliminate the Node variable.

> +/// ValueIsDeleted - This handler is used to update metadata store
> +/// when a value is deleted.
> +void Metadata::ValueIsDeleted(const Instruction *Inst) {
> +  // Find Metadata handles for this instruction.
> +  MDStoreTy::iterator I = MetadataStore.find(Inst);
> +  if (I == MetadataStore.end())
> +    return;

This is a serious logic error if HasMetadata is set on an instruction  
but it doesn't have an entry in MetadataStore.

> +  MDMapTy &Info = I->second;
> +
> +  // FIXME : Give all metadata handlers a chance to adjust.
> +
> +  // Remove the entries for this instruction.
> +  Info.clear();
> +  MetadataStore.erase(Inst);

This should erase(I) to avoid having to do another lookup.

-Chris