[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

[Devang Patel]

Hi Chris

On Sun, Sep 27, 2009 at 3:27 PM, Chris Lattner <clattner at apple.com> wrote:
> 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...

Thanks! I have not checked in doc updates yet.

>> +++ llvm/trunk/include/llvm/Metadata.h Wed Sep 16 13:09:00 2009
>
> +#include "llvm/ADT/SmallSet.h"
>
> This isn't needed, please remove.

ok

>> +//
>> =
>> =
>> =
>> ----------------------------------------------------------------------=
>> ==//
>> +/// 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 :).

ok. How about ContextMetadata? It manages metadata for one llvm context.

> 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".

ok

>> +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.

ok

>
>> +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.

ok

>
>> +  /// 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.

ok

>
> There should also be a "remove" method.

And ReplaceMD. But I see below that you're suggesting to use AddMD to
replace existing entry, if any.

>
>> +
>> +  /// 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.

Right now it uses WeakVH. The goal is to either extend CallBackVH or
write a new VH to handle "clone", "merge" etc..

>> +++ 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.

Right now lexer hardcodes string names, but I'm going to update lexer.
I didn't plan, but l'll  add check here also.

>
>> +  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.

ok

>
>> +  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.

ok

>
>> +/// 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.

ok

>> +/// 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.

ok, I'll put an assert here instead of early return.

>> +  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.

ok

Thanks!
-
Devang