[llvm-commits] embedded metadata preview

[Dan Gohman]

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.

Thanks,

Dan