• Eli who introduced combineMetadataForCSE

Yes, combineMetadataForCSE can be used in more places; I just didn't get around to tracking them all down.

For background, the example I'm looking at:

define i8* @​isa_impl_wrap(i8** %x) {
%t2 = alloca i8*
%t4 = load i8*, i8** %x
store i8* %t4, i8** %t2
%t6 = load i8*, i8** %t2, !nonnull !​0
ret i8* %t6
}
!​0 = !{}

The nonnull disappears with any of these passes, and these all appear to have independent ways of optimizing the code.

$ ./opt -S cse_nonnull.ll -instcombine
...
define i8* @​isa_impl_wrap(i8** %x) {
%t4 = load i8*, i8** %x, align 8
ret i8* %t4
}

$ ./opt -S cse_nonnull.ll -early-cse
...
define i8* @​isa_impl_wrap(i8** %x) {
%t2 = alloca i8*
%t4 = load i8*, i8** %x
store i8* %t4, i8** %t2
ret i8* %t4
}

$ ./opt -S cse_nonnull.ll -mem2reg
...
define i8* @​isa_impl_wrap(i8** %x) {
%t4 = load i8*, i8** %x
ret i8* %t4
}

However, if the nonnull metadata appears on the 1st load only, then it survives all of these passes.

For background, the example I'm looking at:

define i8* @​isa_impl_wrap(i8** %x) {
%t2 = alloca i8*
%t4 = load i8*, i8** %x
store i8* %t4, i8** %t2
%t6 = load i8*, i8** %t2, !nonnull !​0
ret i8* %t6
}
!​0 = !{}

The nonnull disappears with any of these passes, and these all appear to
have independent ways of optimizing the code.

$ ./opt -S cse_nonnull.ll -instcombine
...
define i8* @​isa_impl_wrap(i8** %x) {
%t4 = load i8*, i8** %x, align 8
ret i8* %t4
}

$ ./opt -S cse_nonnull.ll -early-cse
...
define i8* @​isa_impl_wrap(i8** %x) {
%t2 = alloca i8*
%t4 = load i8*, i8** %x
store i8* %t4, i8** %t2
ret i8* %t4
}

$ ./opt -S cse_nonnull.ll -mem2reg
...
define i8* @​isa_impl_wrap(i8** %x) {
%t4 = load i8*, i8** %x
ret i8* %t4
}

However, if the nonnull metadata appears on the 1st load only, then it
survives all of these passes.

It seems like you don't have an available load? Please note that the load you have is dead (EarlyCSE and InstCombine both look at the load and the store immediately preceding it).

For background, the example I'm looking at:

define i8* @​isa_impl_wrap(i8** %x) {
%t2 = alloca i8*
%t4 = load i8*, i8** %x
store i8* %t4, i8** %t2
%t6 = load i8*, i8** %t2, !nonnull !​0
ret i8* %t6
}
!​0 = !{}

See also https://reviews.llvm.org/D27114 .

See also https://reviews.llvm.org/D27114 .

Thanks for the link! I was looking at solving bug 31512 using an assume to attribute transform ( similar to https://reviews.llvm.org/D5951 ), but if we're looking to canonicalize in the other direction, that approach is DOA. :)

OK, I'm starting to understand this slightly more. :)
Sorry for the diversion from the original point of this bug.

Let's look at some possible GVN tests:

define i8 @​nonnull1(i8** %p) {
%a = load i8*, i8** %p, !nonnull !​0
%b = load i8*, i8** %p
%a1 = load i8, i8* %a
%b1 = load i8, i8* %b
%c = add i8 %a1, %b1
ret i8 %c
}

define i8 @​nonnull2(i8** %p) {
%a = load i8*, i8** %p
%b = load i8*, i8** %p, !nonnull !​0
%a1 = load i8, i8* %a
%b1 = load i8, i8* %b
%c = add i8 %a1, %b1
ret i8 %c
}

define i8 @​nonnull3(i8** %p) {
%a = load i8*, i8** %p, !nonnull !​0
%b = load i8*, i8** %p, !nonnull !​0
%a1 = load i8, i8* %a
%b1 = load i8, i8* %b
%c = add i8 %a1, %b1
ret i8 %c
}

Currently, we drop the nonnull in all cases with 'opt -gvn' because MD_nonnull isn't in the list of KnownIDs.

1. Is the ideal behavior to preserve nonnull in all cases?
2. If yes, is that true for all GVN transforms, or is this a special simple case?
3. Should GVN have different behavior than combineMetadataForCSE?

For nonnull in particular, the best way to understand the semantics is probably to translate from !nonnull->assume; then the obvious redundancy rules apply.

For nonnull in particular, the best way to understand the semantics is
probably to translate from !nonnull->assume; then the obvious redundancy
rules apply.

Please correct me if I'm misunderstanding, but if it's easier to reason about using assumes, that's another vote to reverse the current instcombine canonicalization.

I like the compactness of the metadata, but given how easily it is lost, I'm seeing the advantage of the assume now.

Sort of, yes... I mean, nonnull isn't really a property of the load; it's a property of the produced value, so attaching it to the load doesn't really make sense. But llvm.assume has a terrible design which tends to screw up other optimizations, so nobody really wants to use it.

The BBVectorizer was removed from trunk:
https://reviews.llvm.org/rL306797