[llvm-commits] embedded metadata preview

[Nick Lewycky]

Chris Lattner wrote:
>>> The parser parts are not present in the patch.  That may be a reason
>>> why it wasn't clear why Constant is used.
>> That was accidental. They don't turn up in 'svn diff' unless I  
>> explicitly ask for 'svn diff lib/AsmParser'.
>>
>> Here, I've attached a newer version of the patch which ought to  
>> include everything.
> 
> Hi Nick,
> 
> I'm really sorry for the delay in review.  Thank you for working on  
> it!  This work should be a host to a whole class of new coolness in  
> llvm, including debug info improvements and TBAA stuff.  I'm excited  
> that you're pushing this forward!

No problem. Thanks for the review! I'm looking forward to seeing what 
people make of embedded metadata.

> +++ include/llvm/Type.h	(working copy)
> @@ -326,7 +326,7 @@
>     // 
> ===-------------------------------------------------------------------- 
> ===//
>     // These are the builtin types that are always available...
>     //
> -  static const Type *VoidTy, *LabelTy, *FloatTy, *DoubleTy;
> +  static const Type *VoidTy, *LabelTy, *FloatTy, *DoubleTy,  
> *MetadataTy;
>     static const Type *X86_FP80Ty, *FP128Ty, *PPC_FP128Ty;
>     static const IntegerType *Int1Ty, *Int8Ty, *Int16Ty, *Int32Ty,  
> *Int64Ty;
> 
> For now (unless MD gets its own type), this should probably be named  
> EmptyStructTy.

Done.

> +class MDString : public Constant {
> 
> +  explicit MDString(const std::vector<Constant*> &Val);
> 
> ctor is dead, plz remove.

Done.

> +  /// length() - The length of this string.
> +  ///
> +  unsigned length() const { return StrEnd - StrBegin; }
> 
> how about "size()" instead of length() ?

Sure. Done.

> +// 
> = 
> = 
> =---------------------------------------------------------------------- 
> ===//
> +/// MDNode - a tuple of other values.
> +/// These are used to efficiently contain a byte sequence for metadata.
> 
> I don't think "byte sequence" is accurate here.

Oops! Fixed. New text reads:

/// MDNode - a tuple of other values.
/// These contain a list of the Constants that represent the metadata.

> +class MDNode : public Constant, public FoldingSetNode {
> +  friend struct ConstantCreator<MDNode, Type, std::vector<Constant*> >;
> 
> I think the "friend" is dead?
> 
> +  static MDNode *get(const std::vector<Constant*> &V) {
> +    if (V.empty())
> +      return get(NULL, 0);
> +    else
> +      return get(&*V.begin(), V.size());
> +  }
> 
> Can we just remove this ctor and force clients to do this?

Done. Turns out the only user of it already knew the vector would never 
be empty.

> +  /// getType() specialization - Type is always void.
> +  ///
> +  inline const Type *getType() const {
> +    return Type::MetadataTy;
> 
> s/void/{}/ in comment.

Oops! Fixed.

> +++ lib/VMCore/Constants.cpp	(working copy)
> 
> +static StringMap<MDString*> MDStringCache;
> +static FoldingSet<MDNode> MDNodeSet;
> 
> These should use ManagedStatic<> so that the map/set is lazily created  
> and destroyed on llvm_shutdown()

Done.

> +void MDNode::replaceUsesOfWithOnConstant(Value *From, Value *To, Use  
> *U) {
> +  assert(isa<Constant>(To) && "Cannot make Constant refer to non- 
> constant!");
> +
> +  std::vector<Constant*> Values;
> 
> 
> How about using a SmallVector<x,8> here?

Done.

Note that this is a copy'n'paste of 
ConstantVector::replaceUsesOfWithOnConstant which I did not update 
because its getter takes std::vector explicitly. (It has a ptr+size 
constructor, but that one doesn't take the Type*.)

> +++ lib/AsmParser/LLToken.h	(working copy)
> @@ -115,6 +115,9 @@
>       LocalVar,          // %foo %"foo"
>       StringConstant,    // "foo"
> 
> +    // Metadata valued tokens.
> +    Metadata,          // !42 !"foo"
> 
> 
> Does the parser support the !42 syntax?

Nope. I changed the example to // !"foo" !{i8 42}

> +++ lib/Bitcode/Reader/BitcodeReader.cpp	(working copy)
> 
> 
> +    case bitc::CST_CODE_MDSTRING: {
> +      if (Record.size() < 2) return Error("Invalid MDSTRING record");
> +      unsigned MDStringLength = Record.size();
> +      char *String = new char[MDStringLength];
> 
> instead of new[], please use something like:
> 
> SmallString<8> String;
> String.resize(MDStringLength);
> ...

Ooh, SmallString! Done deal.

By the way, SmallString lets me do this:

   SmallString<8> String(Record.begin(), Record.end());

with no type error. As far as I can tell, this would cause the wrong 
thing to happen at run time.

> +    case bitc::CST_CODE_MDNODE: {
> +      if (Record.empty() || Record.size() % 2 == 1)
> +        return Error("Invalid CST_MDNODE record");
> +
> +      unsigned Size = Record.size();
> +      std::vector<Constant*> Elts;
> 
> SmallVector plz.

Done.

> +++ docs/LangRef.html	(working copy)
> 
> +empty struct and is identified in syntax by a preceeding exclamation  
> point
> 
> preceeding -> preceding

Doh. Fixed.

> After reading through the patch, I completely agree that introducing a  
> new "metadatatype" is the right direction to go for.
> 
> Please apply with these changes,

Will do as soon as my nightly comes back clean!

Nick