[llvm-commits] embedded metadata preview

[Nick Lewycky]

Ping!

Chris, you asked to review this before I committed it, since it is a 
large blob of new code. I know you're busy, but I'd appreciate being 
able to get it out of my tree :)

See attachment here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20090316/075433.html
no need to fill everyone's inboxes with it again.

Nick

Nick Lewycky wrote:
> 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