llvm dev - Dec 2014 - [LLVMdev] Removing types from metadata

On Fri, Dec 19, 2014 at 12:56 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> However, I think this would set a bad precedent.  There's nowhere else
> (that I know of) where we accept two versions of assembly.  The
> LLParser is relatively easy to work with because it doesn't have that
> kind of historical baggage.

I can think of two precedents: attribute syntax and linker_private vs
private. For attributes, we still parse the old syntax with the builtin
non-string attributes after the ')' token, but that may be because
it's
just more readable. linker_private is still there because it's trivial to
support.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20141219/454653a6/attachment.html>

> IMO, that's the only compelling reason.  Same reason that you can
define
> metadata like this:
>
> define void @foo(i1 %v) {
>   br i1 %v, label %t, label %f, !prof !{"branch_weights", i32 1,
i32 2}
> t:
>   ret void
> f:
>   ret void
> }
>
> Instead of:
>
> define void @foo(i1 %v) {
> entry:
>   br i1 %v, label %true, label %false, !prof !0
> true:
>   ret void
> false:
>   ret void
> }
> !0 = !{"branch_weights", i32 1, i32 2}
>
>> linker_private is still there because it's trivial to support.
>
> Actually, that's only around for bitcode compatibility tests:
>
> $ git grep linker_private
> lib/Target/NVPTX/NVPTXAsmPrinter.cpp:// internal, private, linker_private,
> lib/Target/NVPTX/NVPTXAsmPrinter.cpp:// linker_private_weak,
linker_private_weak_def_auto,
> test/Bitcode/linkage-types-3.2.ll:@linker_private.var = linker_private
constant i32 0
> test/Bitcode/linkage-types-3.2.ll:; CHECK: @linker_private.var = private
constant i32 0
> test/Bitcode/linkage-types-3.2.ll:@linker_private_weak.var =
linker_private_weak constant i32 0
> test/Bitcode/linkage-types-3.2.ll:; CHECK: @linker_private_weak.var =
private constant i32 0
> test/Bitcode/linkage-types-3.2.ll:@linker_private_weak_def_auto.var =
linker_private_weak_def_auto constant i32 0
> test/Bitcode/linkage-types-3.2.ll:; CHECK:
@linker_private_weak_def_auto.var = constant i32 0
> test/Bitcode/linkage-types-3.2.ll:define linker_private void
@linker_private()
> test/Bitcode/linkage-types-3.2.ll:; CHECK: define private void
@linker_private
> test/Bitcode/linkage-types-3.2.ll:define linker_private_weak void
@linker_private_weak()
> test/Bitcode/linkage-types-3.2.ll:; CHECK: define private void
@linker_private_weak
> test/Bitcode/linkage-types-3.2.ll:define linker_private_weak_def_auto void
@linker_private_weak_def_auto()
> test/Bitcode/linkage-types-3.2.ll:; CHECK: define void
@linker_private_weak_def_auto
>
> Looking back through git history, I see that it was kept around for a
> deprecation period (I vaguely remember the thread).
>
>   - Rafael removed it in r203866.
>   - Saleem restored it in r205675.
>   - Saleem added deprecation warnings in r205681.
>   - Saleem removed it in r213777 (after 3.5 branch).
>
> While I'm only slightly opposed to this in general (assuming it gets
> ripped out shortly), I'm quite opposed to it for this change.
>
> IMO, it's important that the three forms of IR match each other
> closely -- most of all, assembly and C++.  The metadata/value split is
> most difficult because of C++ API changes; I think the assembly
> changes are relatively minor and easy enough to script.  (I
> particularly don't want to maintain parser support for
> `metadata !{i32 %val}`, which pretends that `i32 %val` is stored in
> an `MDNode`.  It was a huge cleanup to remove that from the parser.)
>
> What do others think, though?
I am also in favor of having a single upgrade path: bitcode.

Cheers,
Rafael