[llvm-commits] embedded metadata preview

[Dan Gohman]

Hi Nick,

Here are a few review comments.

 > Index: lib/Bitcode/Writer/BitcodeWriter.cpp
 > ===================================================================
 > --- lib/Bitcode/Writer/BitcodeWriter.cpp        (revision 66999)
 > +++ lib/Bitcode/Writer/BitcodeWriter.cpp        (working copy)
 > @@ -677,6 +677,17 @@
 >          Record.push_back(CE->getPredicate());
 >          break;
 >        }
 > +    } else if (const MDString *S = dyn_cast<MDString>(C)) {
 > +      Code = bitc::CST_CODE_MDSTRING;
 > +      Record.push_back(S->length());
 > +      for (unsigned i = 0, e = S->length(); i != e; ++i)
 > +        Record.push_back(S->begin()[i]);

It seems pretty inefficient to put each char in a uint64_t.
Forgive me for prematurely optimizing :-).

 > Index: lib/Bitcode/Reader/BitcodeReader.cpp
 > ===================================================================
 > --- lib/Bitcode/Reader/BitcodeReader.cpp        (revision 66999)
 > +++ lib/Bitcode/Reader/BitcodeReader.cpp        (working copy)
 > @@ -283,10 +283,12 @@
 >                                     UserCS->getType()->isPacked());
 >        } else if (isa<ConstantVector>(UserC)) {
 >          NewC = ConstantVector::get(&NewOps[0], NewOps.size());
 > -      } else {
 > -        // Must be a constant expression.
 > +      } else if (isa<ConstantExpr>(UserC)) {
 >          NewC = cast<ConstantExpr>(UserC)- 
 >getWithOperands(&NewOps[0],
 >                                                             
NewOps.size());
 > +      } else {
 > +        // Must be a metadata node.
 > +        NewC = MDNode::get(&NewOps[0], NewOps.size());
 >        }
 >

Can you add an assert(isa<MDNode>) here to verify the assertion in
the comment?

 > Index: docs/LangRef.html
 > ===================================================================
 > --- docs/LangRef.html   (revision 66999)
 > +++ docs/LangRef.html   (working copy)

 > @@ -1847,6 +1848,14 @@
 >    large arrays) and is always exactly equivalent to using explicit  
zero
 >    initializers.
 >    </dd>
 > +
 > +  <dt><b>Metadata node</b></dt>
 > +
 > +  <dd>A metadata node is a structure-like constant with the type  
of an empty
 > +  struct.  For example: "<tt>{ } !{ i32 0, { } !"test" }</tt>".  
Unlike other
 > +  constants that are meant to be interpreted as part of the  
instruction stream,
 > +  metadata is a place to attach additional information such as  
debug info.

It would be helpful to mention that this is also expected to be used for
encoding information that will be used by optimizations.


MDString and MDNode have protected constructors.  Do you anticipate
these classes having subclasses? If so, what would they be like?

Why do MDString and MDNode inherit from Constant instead of, say,  
User? I
see that ConstantLastVal is moved to include MDStringVal and MDNodeVal,
presumably for the same reason.

Dan

On Mar 14, 2009, at 1:45 AM, Nick Lewycky wrote:

> I took a stab at implementing embedded metadata. The idea is to  
> allow LLVM's user to attach meaningful data to the instruction  
> stream that isn't really relevant to the execution of the  
> instructions. This comes from Chris' notes:
>
>  http://nondot.org/sabre/LLVMNotes/EmbeddedMetadata.txt
>
> which I deviated a little. The major thing missing from this patch  
> is any verifier support. My current plan is to add support to the  
> verifier for ensuring that MDNode and MDString are only used from a)  
> initializers to globals named starting with "llvm." b) calls to  
> intrinsic functions, then commit it.
>
> The attached patch implements MDString and MDNode as new types of  
> Constant, .ll/.bc reader/writer support and a LangRef update. I'd  
> appreciate any review comments!
>
> Nick
> <emb-md.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits