[PATCH] IR: Make metadata typeless in assembly

[Duncan P. N. Exon Smith]

Okay, so I went right down the rabbit hole with this, and I've attached
a new patch that skips the `value` keyword.  I have a new upgrade script
that gets all the LLVM tests passing.

The end result looks obviously better to me.  I wasn't sure if there'd
be ambiguities between complex types (starting with `%{<[`) and other
metadata operands, but it looks like all metadata operands either start
with `!` or are `null`.  This adds a restriction on syntax for new
types of metadata to be unambiguous w.r.t. types, but I don't think
that's serious.

Haven't attached the upgrade script or massive test patch here (although
I have updated LangRef as per Eric's suggestions).

Paul, let me know if this looks better to you too.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: metadata-value-split-assembly-3.patch
Type: application/octet-stream
Size: 28343 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141211/505d11f4/attachment.obj>
-------------- next part --------------


> On 2014 Dec 11, at 16:26, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> Actually, maybe we don't need that after all.
> 
> I had expected to need the `value` keyword to detect whether the operand
> would take a type.  I'm not so sure now that I've written the parser (and
> the nice errors about invalid types).  I'll see what I can do; give me a
> few minutes.
> 
> 
>> On 2014 Dec 11, at 16:16, Robinson, Paul <Paul_Robinson at playstation.sony.com> wrote:
>> 
>> Duncan,
>> Remind me why it's important to have the 'value' keyword?  This bikeshed was
>> probably painted a while back but syntactically it doesn't seem too useful.
>> It seems like you have (a) stuff starting with '!' is a metadata node/reference,
>> (b) everything else is a value (and must start with a type name).  There's
>> probably something I'm forgetting, that made 'value' useful…
>> Thanks,
>> --paulr
>> 
>> 
>> From: Eric Christopher [mailto:echristo at gmail.com] 
>> Sent: Thursday, December 11, 2014 4:07 PM
>> To: Duncan P. N. Exon Smith; Robinson, Paul
>> Cc: llvm-commits
>> Subject: Re: [PATCH] IR: Make metadata typeless in assembly
>> 
>> Hi Duncan,
>> 
>> Looking through the LangRef as requested :)
>> 
>> +    A metadata node is a structure-like constant without a type (when
>> +    referenced by ``call`` instructions, they use :ref:`metadata type
>> +    <t_metadata>`). For example: "``!{ value i32 0, !"test" }``". 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.
>> 
>> This is fairly opaque wording. Honestly I'd prefer "a constant tuple without types". The last probably needs/wants rewording as well, it's just not fabulous. Perhaps even an actual separate section would work well for metadata now that it's separate from the value hierarchy?
>> 
>> The rest of the changes look fairly mechanical given the new description of the format.
>> 
>> Thanks!
>> 
>> -eric
>> 
>> 
>> On Wed Dec 10 2014 at 10:42:34 PM Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>>> On 2014 Dec 10, at 20:35, Robinson, Paul <Paul_Robinson at playstation.sony.com> wrote:
>>> 
>>> Note: in case it's confusing, some of the testcase changes in the code
>>> patch are killing newlines so the upgrade script can do it's magic.
>>> 
>>> That part really feels like a purely mechanical change that oughta be separate.
>>> --paulr
>> 
>> Thanks Paul, good point.  Committed that in r224002.
>> 
>> Attaching the rebased patch.  Let me know if you have any comments on LangRef!
>> 
>> 
>>> 
>>> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Duncan P. N. Exon Smith
>>> Sent: Wednesday, December 10, 2014 6:17 PM
>>> To: llvm-commits
>>> Subject: [PATCH] IR: Make metadata typeless in assembly
>>> 
>>> Now that `Metadata` is typeless, reflect that in the assembly.
>>> 
>>> There's really only one reasonably thing to do here: strip the
>>> `metadata` type (except in arguments to `call` instructions, where it
>>> still has meaning), and add the symmetric `value` keyword for all
>>> metadata operands that bridge to `Value`.
>>> 
>>> I'm confident in the code changes (although obviously I'll relish any
>>> criticism offered), but I'd love a review of `docs/LangRef.rst`.  I
>>> wasn't really sure how much exposition was appropriate, so I kept it
>>> pretty simple.
>>> 
>>> Also, I wrote an upgrade script that handle all of the tests in llvm
>>> after applying the main patch.  It even handles much of cfe (doing
>>> mostly the right thing with `CHECK` lines).  I'll be attaching it the PR
>>> after I make my commit, but feel free to spot-check its enormous result,
>>> which I've attached as llvm-tests.patch.  (I'll squash that in with the
>>> code changes when I commit.)
>>> 
>>> Note: in case it's confusing, some of the testcase changes in the code
>>> patch are killing newlines so the upgrade script can do it's magic.
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>