Removing types from metadata

Hi,

Types were recently removed from metadata in the IR, which means older
LLVM IR code with metadata won't compile. Was this intentional?
Is there a practical way to maintain compatibility with older LLVM IR?

Thanks,
Tom

Backwards compatibility with the LLVM IR textual format is not guaranteed, while bitcode compatibility is. You can probably load up that old IR by assembling it with an old llvm-as and loading the bitcode in new clang.
http://llvm.org/docs/DeveloperPolicy.html#ir-backwards-compatibility

That said, if it’s just a matter of skipping over the ‘metadata’ token, it would be nice to let old IR assemble.

I’m assuming you’re referring to http://llvm.org/viewvc/llvm-project?view=revision&revision=224257 - certainly deliberate. +Duncan for more info.

Hi,

Types were recently removed from metadata in the IR, which means older
LLVM IR code with metadata won't compile. Was this intentional?
Is there a practical way to maintain compatibility with older LLVM IR?

Thanks,
Tom

As Reid said, there's no backwards compatibility for assembly. However,
I did share an upgrade script (as mentioned in r224257) that should help
you update out-of-tree testcases (or other IR). Have a look at the
attachment to PR21532.

Warning: Reid noticed that the `sed -E` flag I'm using there doesn't
seem to be portable. You might have to fiddle with the regexes (please
share if you do).

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.

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?

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

Me too.

-Chris