[llvm-commits] embedded metadata preview

[Nick Lewycky]

Dan Gohman wrote:
> On Mar 16, 2009, at 10:29 PM, Nick Lewycky wrote:
> 
>> Dan Gohman wrote:
>>> 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 :-).
>> No that's fine, I just copied this out of the handling for strings in
>> inline-asm under the assumption that it must be fine.
>>
>> I tried adding "AbbrevToUse = String8Abbrev;" here but that causes a
>> crash ("Invalid abbrev for record"). I'm not really familiar with this
>> code, do you know how it's supposed to work? (If not I'm sure I can
>> figure it out...)
> 
> I'm not familiar with this code either.
> 
>>>> 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.
>> I'm actually thinking of removing any mention of them here and only
>> talking about them under the "Embedded Metadata" section. Thoughts?
>>
>> I added this as the last paragraph in that section:
>>   "Optimizations may rely on metadata to provide additional  
>> information
>> about the program that isn't available in the instructions, or that
>> isn't easily computable. Similarly, the code generator may expect a
>> certain metadata format to be used to express debugging information."
>>
>> Close enough? :)
> 
> Sounds reasonable.
> 
>>> 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.
>> I tried that initially, where MDNode was a User and MDString was a
>> Value. In practise, they seem to have semantics much closer to that of
>> Constants; they're uniqued / pointer comparable and they're global.  
>> The
>> point where it really made a difference was in the .ll parser,  
>> switching
>> them over to being Constant made the parser changes much simpler.
> 
> 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.

Thanks for the review!

Nick

> Thanks,
> 
> Dan
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: emb-md2.patch
Type: text/x-diff
Size: 30860 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090318/9b7bd177/attachment.patch>