[LLVMdev] Removing types from metadata

[Duncan P. N. Exon Smith]

> On 2014-Dec-19, at 13:02, Reid Kleckner <rnk at google.com> wrote:
> 
> 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.

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?