[RFC] Semantic changes in the Metadata/Value split

Project Infrastructure LLVM Dev List Archives
1

As of Monday, I finally got a preliminary patch passing check and
check-clang with the metadata-value split.

It's rather massive, but mostly NFC.

I've managed to keep the bitcode format, assembly syntax, and C API
unchanged, except for a few semantic restrictions that I'll separate out
and commit ahead of time via assertions to the extent I can. (Obviously
I plan to clean up assembly and bitcode in a follow-up commit.)

This is mainly a heads up about what the semantic changes are; I think
they're mostly unavoidable given the constraints.

  - Function-local metadata is restricted to direct arguments to call
    intrinsics (i.e., `metadata !{i32 %val}`). Temporarily, it's
    spelled the same way as an `MDNode` in assembly.

  - Except during "construction" -- for uniquing forward declarations --
    `MDNode` does not support `replaceAllUsesWith()`.

  - When an `MDNode` operand changes and there's a uniquing collision,
    uniquing is dropped for that node -- it becomes "distinct". (Note
    that an operand going to null no longer turns off uniquing, unless
    there's a collision. `MDNode` now supports `dropAllReferences()`,
    so there's no more web of RAUW during teardown of the context.)

  - Nodes that self-reference aren't uniqued at all.

I've attached my current patch (rebased last night) as a reference.
I'll separate out as much as I can before committing the main thing.

Aside from adding assertions for the above-mentioned semantic changes,
if anyone sees anything obvious to separate out let me know.

There are also a few follow-ups that I've left until later:

  - Change assembly syntax to make metadata look as typeless as it is.
    (This will come with painful clang testcase updates... yay!)

  - Ditto for bitcode.

  - Add assembly syntax, bitcode support, and C++ API for
    `MDNode::getDistinct()`, which explicitly opts out of uniquing.

  - Add `MDInt`, which is a generic integer represented by an `APSInt`
    for use in metadata.

  - Upgrade various types of metadata to use `MDInt` and/or
    `getDistinct()`.

  - Add reference counting semantics to some types of metadata, so that
    no-longer-referenced metadata gets cleaned up.

  - Upgrade `DebugLoc` from 8 bits of column info to 16.

  - ... and get back to the debug info IR changes that get me into this
    mess :).

(I actually tried really hard to get half of this working without the
other half. I created the `ValueAsMetadata` bridge and restricted
`MDNode` to only have subclasses of `Metadata` as operands, but left
`Metadata` inheriting from `Value`. This only took a few days to get
compiling, but left tons of errors to be found at runtime due to failing
`cast<>` failures. Although there are small things that I can break
off, IMO fundamentally splitting up this patch is untenable.)

metadata-value-split.patch (342 KB)

metadata-value-split-clang.patch (67.4 KB)

2

Do you have the Go bindings enabled? Because of the changes you made to the
DIBuilder interface, I expect that your changes will break the bindings.

I'm happy to help with updating the bindings.

Thanks,

3

As of Monday, I finally got a preliminary patch passing check and
check-clang with the metadata-value split.

Do you have the Go bindings enabled? Because of the changes you made to the
DIBuilder interface, I expect that your changes will break the bindings.

Yes, that's probably true :(.

There aren't really any DIBuilder changes, but the split itself will
probably cause compile failures. I expect the fixes will be fairly
mechanical, along the lines of the changes I made to lib/IR/Core.cpp.

I don't see instructions for enabling the bindings on the CMake page [1].
Is there a way to do it?

[1]: http://llvm.org/docs/CMake.html

(I can compile the bindings on Darwin, right?)

I'm happy to help with updating the bindings.

Thanks!

4

>
>> As of Monday, I finally got a preliminary patch passing check and
>> check-clang with the metadata-value split.
>
> Do you have the Go bindings enabled? Because of the changes you made to the
> DIBuilder interface, I expect that your changes will break the bindings.

Yes, that's probably true :(.

There aren't really any DIBuilder changes, but the split itself will
probably cause compile failures. I expect the fixes will be fairly
mechanical, along the lines of the changes I made to lib/IR/Core.cpp.

Right.

I don't see instructions for enabling the bindings on the CMake page [1].
Is there a way to do it?

[1]: http://llvm.org/docs/CMake.html

They should be enabled automatically if you have Go installed (or more
specifically, if you have "go" in your $PATH). There are installation packages
available at:

(I can compile the bindings on Darwin, right?)

I made sure the bindings worked on Darwin before checking them in, so you
should be able to compile them.

Thanks,

5

The attached patch gets the Go test passing. I'll include it as part of
my commit.

Thanks for the help!

go-bindings.patch (7.29 KB)

6

LGTM, thanks.

Eventually we should be able to expose the Metadata type directly in the Go
bindings, but that will involve more extensive changes to the bindings and
its clients. I'll try to do that at some point after this is checked in.

Thanks,

7

As of Monday, I finally got a preliminary patch passing check and
check-clang with the metadata-value split.

Do you have the Go bindings enabled? Because of the changes you made to the
DIBuilder interface, I expect that your changes will break the bindings.

Yes, that's probably true :(.

There aren't really any DIBuilder changes, but the split itself will
probably cause compile failures. I expect the fixes will be fairly
mechanical, along the lines of the changes I made to lib/IR/Core.cpp.

Right.

I don't see instructions for enabling the bindings on the CMake page [1].
Is there a way to do it?

[1]: http://llvm.org/docs/CMake.html

They should be enabled automatically if you have Go installed (or more
specifically, if you have "go" in your $PATH). There are installation packages
available at:

Downloads - The Go Programming Language

(I can compile the bindings on Darwin, right?)

I made sure the bindings worked on Darwin before checking them in, so you
should be able to compile them.

The attached patch gets the Go test passing. I'll include it as part of
my commit.

Thanks for the help!

LGTM, thanks.

Eventually we should be able to expose the Metadata type directly in the Go
bindings, but that will involve more extensive changes to the bindings and
its clients. I'll try to do that at some point after this is checked in.

Yup, that makes sense to me. I think the first step would be to expose
it in include/llvm-c/Core.h.

8

As of Monday, I finally got a preliminary patch passing check and
check-clang with the metadata-value split.

Do you have the Go bindings enabled? Because of the changes you made to the
DIBuilder interface, I expect that your changes will break the bindings.

Yes, that's probably true :(.

There aren't really any DIBuilder changes,

Glad to hear that. I was just finishing up C and Modula3 bindings to DIBuilder.

but the split itself will