[llvm-commits] [llvm] r92931 - in /llvm/trunk: include/llvm/Metadata.h include/llvm/Module.h include/llvm/ValueSymbolTable.h lib/Bitcode/Writer/BitcodeWriter.cpp lib/Bitcode/Writer/ValueEnumerator.cpp lib/Bitcode/Writer/ValueEnumerator.h lib/VMCore/Metadata.cpp lib/VMCore/Module.cpp test/Feature/NamedMDNode.ll

[Chris Lattner]

On Jan 7, 2010, at 11:39 AM, Devang Patel wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=92931&view=rev
> Log:
> Use separate namespace for named metadata.

Great!

> +++ llvm/trunk/include/llvm/Metadata.h Thu Jan  7 13:39:36 2010

> +  /// setName - Set the name of this named metadata.
> +  void setName(StringRef Name);

If setName is part of the public API for this class then it should ensure that the module symtab is kept up to date.  Right now, setName will change the NamedMDNode but not the symtab afaict.

> +++ llvm/trunk/include/llvm/Module.h Thu Jan  7 13:39:36 2010
> @@ -322,6 +324,10 @@
>   /// NamedMDNode with the specified name is not found.
>   NamedMDNode *getOrInsertNamedMetadata(StringRef Name);
> 
> +  /// addMDNodeName - Insert an entry in the NamedMDNode symbol table mapping
> +  /// Name to NMD. 
> +  void addMDNodeName(StringRef Name, NamedMDNode *NMD);

This should not be public API.

> +++ llvm/trunk/include/llvm/ValueSymbolTable.h Thu Jan  7 13:39:36 2010
> @@ -129,6 +129,84 @@
> /// @}
> };
> 
> +/// This class provides a symbol table of name/NamedMDNode pairs. It is 
> +/// essentially a StringMap wrapper.
> +
> +class MDSymbolTable {
> +/// @name Types
> +/// @{
> +public:
> +  /// @brief A mapping of names to metadata
> +  typedef StringMap<NamedMDNode*> MDMap;

This typedef should not be public.  Please also disable copy ctor and operator=.

> +public:
> +  /// insert - The method inserts a new entry into the stringmap.
> +  void insert(StringRef Name,  NamedMDNode *Node) {
> +    (void) mmap.GetOrCreateValue(Name, Node);
> +  }
> +  
> +  /// This method removes a NamedMDNode from the symbol table.  
> +  void remove(StringRef Name) { mmap.erase(Name); }

Should these be private?  Random classes should interact with the symbol table through Module and the NamedMDNode constructor.

> +++ llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp Thu Jan  7 13:39:36 2010
> @@ -528,10 +528,9 @@
>       }
> 
>       // Write name.
> -      std::string Str = NMD->getNameStr();
> -      const char *StrBegin = Str.c_str();
> -      for (unsigned i = 0, e = Str.length(); i != e; ++i)
> -        Record.push_back(StrBegin[i]);
> +      StringRef Str = NMD->getName();
> +      for (unsigned i = 0, e = Str.size(); i != e; ++i)
> +        Record.push_back(Str[i]);
>       Stream.EmitRecord(bitc::METADATA_NAME, Record, 0/*TODO*/);
>       Record.clear();

This is a fine change.  However, coming back to the bitcode encoding of NamedMDNodes, why is this split into two records?  It seems that METADATA_NAME / METADATA_NAMED_NODE could be one record.

The first element of the record would be the # MDNodes, then that # of mdnode id's would follow, then a length of the name, then the characters of the name.

> +++ llvm/trunk/lib/VMCore/Metadata.cpp Thu Jan  7 13:39:36 2010
> @@ -214,20 +214,21 @@
>   return *(SmallVector<WeakVH, 4>*)Operands;
> }
> 
> -NamedMDNode::NamedMDNode(LLVMContext &C, const Twine &N,
> +NamedMDNode::NamedMDNode(LLVMContext &C, StringRef N,

Shouldn't this take a Twine, not a StringRef for consistency with everything else?

>                          MDNode *const *MDs, 
>                          unsigned NumMDs, Module *ParentModule)
>   : MetadataBase(Type::getMetadataTy(C), Value::NamedMDNodeVal), Parent(0) {
>   setName(N);
> -    
>   Operands = new SmallVector<WeakVH, 4>();
> 
>   SmallVector<WeakVH, 4> &Node = getNMDOps(Operands);
>   for (unsigned i = 0; i != NumMDs; ++i)
>     Node.push_back(WeakVH(MDs[i]));
> 
> -  if (ParentModule)
> +  if (ParentModule) {
>     ParentModule->getNamedMDList().push_back(this);
> +    ParentModule->addMDNodeName(N, this);

If you set up the ilist_traits right, you don't need to do this.  The traits should autoinsert the NamedMDNode into the symbol table when it is added to a module, and autoremove it from the symtab when it is removed from the module.

> @@ -265,6 +266,7 @@
> /// eraseFromParent - Drop all references and remove the node from parent
> /// module.
> void NamedMDNode::eraseFromParent() {
> +  getParent()->getMDSymbolTable().remove(getName());
>   getParent()->getNamedMDList().erase(this);

Likewise.

> }
> 
> @@ -273,6 +275,16 @@
>   getNMDOps(Operands).clear();
> }
> 
> +/// setName - Set the name of this named metadata.
> +void NamedMDNode::setName(StringRef N) {
> +  if (!N.empty())
> +    Name = N.str();

Should assert that N != empty.  All NamedMDNodes must have non-empty names.

> +++ llvm/trunk/lib/VMCore/Module.cpp Thu Jan  7 13:39:36 2010
> @@ -59,6 +59,7 @@
>   : Context(C), ModuleID(MID), DataLayout("")  {
>   ValSymTab = new ValueSymbolTable();
>   TypeSymTab = new TypeSymbolTable();
> +  NamedMDSymTab = new MDSymbolTable();

You added a new, but not a delete to the destructor.  Please don't leak this.

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100108/3817fe56/attachment.html>