[llvm-commits] embedded metadata preview

[Chris Lattner]

>> 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!

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



+class MDString : public Constant {

+  explicit MDString(const std::vector<Constant*> &Val);

ctor is dead, plz remove.


+  /// length() - The length of this string.
+  ///
+  unsigned length() const { return StrEnd - StrBegin; }

how about "size()" instead of length() ?


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


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

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

s/void/{}/ in comment.


+++ 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()

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




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



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


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

+++ docs/LangRef.html	(working copy)

+empty struct and is identified in syntax by a preceeding exclamation  
point

preceeding -> preceding



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,

-Chris