Mark &mut pointers as noalias once LLVM no longer miscompiles them

[bstrie]

This issue tracks the undoing of the workaround introduced in #31545 on account of a bug in LLVM. cc @dotdash

[jplatte]

Was just reading about this, and it was a little hard to find the LLVM bug report, so for reference, here it is.

[jplatte]

Was just reading about this, and it was a little hard to find the LLVM bug report, so for reference, here it is.

[eefriedman]

I've been working through the LLVM optimization passes to find noalias-related bugs. Known issues:

• https://bugs.llvm.org/show_bug.cgi?id=25422 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27849 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27857 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27858 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27859 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27860 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27952 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27953 Fixed

Some of these are less important because they can't caused a wrong value, only a segfault.

There are probably a few more lurking issues, but hopefully I've found most of them.

---

(Edited by bluss 2016-12-15 to strike out fixed bug)
(Edited by mbrubeck 2017-03-30 to link to new LLVM bugzilla domain)
(Edited by scottmcm 2018-03-29 to strike out fixed bug)

[eefriedman]

I've been working through the LLVM optimization passes to find noalias-related bugs. Known issues:

• https://bugs.llvm.org/show_bug.cgi?id=25422 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27849 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27857 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27858 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27859 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27860 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27952 Fixed
• https://bugs.llvm.org/show_bug.cgi?id=27953 Fixed

Some of these are less important because they can't caused a wrong value, only a segfault.

There are probably a few more lurking issues, but hopefully I've found most of them.

---

(Edited by bluss 2016-12-15 to strike out fixed bug)
(Edited by mbrubeck 2017-03-30 to link to new LLVM bugzilla domain)
(Edited by scottmcm 2018-03-29 to strike out fixed bug)

[mrhota]

@eefriedman LLVM bug 27859 is closed.

[mrhota]

@eefriedman LLVM bug 27859 is closed.

[gnzlbg]

@eefriedman bug 25422 should be fixed.

Any progress on bug 27860 ?

---

(Edited by mbrubeck 2017-03-30 to link to new LLVM bugzilla domain)

[gnzlbg]

@eefriedman bug 25422 should be fixed.

Any progress on bug 27860 ?

---

(Edited by mbrubeck 2017-03-30 to link to new LLVM bugzilla domain)

[pmarcelll]

NewGVN was recently merged into LLVM (still experimental), it's a rewrite of the global value numbering algorithm. The last remaining bug on our list is bug in the old gvn implementation. I compiled the example codes in the bug report with the new gvn algorithm, and they work fine, so hopefully LLVM 5.0 will stabilize NewGVN and we can turn this optimization back on.
EDIT: NewGVN integration into LLVM tracked here

[pmarcelll]

NewGVN was recently merged into LLVM (still experimental), it's a rewrite of the global value numbering algorithm. The last remaining bug on our list is bug in the old gvn implementation. I compiled the example codes in the bug report with the new gvn algorithm, and they work fine, so hopefully LLVM 5.0 will stabilize NewGVN and we can turn this optimization back on.
EDIT: NewGVN integration into LLVM tracked here

[jrmuizel]

I talked with Davide Italiano from LLVM and the goal is for NewGVN to be turned on in LLVM 6.0.
Given that and the fact that https://bugs.llvm.org//show_bug.cgi?id=27860 also affects code that doesn't use restrict/noalias I wonder if we could try turning this on? Even if we could only use this when compiling in panic=abort mode (as it seems many of the codegen issues were related to exceptions).

#42047 is a reduced test case of a real performance problem that's helped by this that we're running into with WebRender so it would be nice to fix this sooner rather than later.

[jrmuizel]

I talked with Davide Italiano from LLVM and the goal is for NewGVN to be turned on in LLVM 6.0.
Given that and the fact that https://bugs.llvm.org//show_bug.cgi?id=27860 also affects code that doesn't use restrict/noalias I wonder if we could try turning this on? Even if we could only use this when compiling in panic=abort mode (as it seems many of the codegen issues were related to exceptions).

#42047 is a reduced test case of a real performance problem that's helped by this that we're running into with WebRender so it would be nice to fix this sooner rather than later.

[Vurich]

I support enabling this if and only if panic = "abort". Even if having the codegen be different depending on panic mode is not currently supported it would be something deserving of an RFC in my opinion, since there are a bunch of possible optimisations that this could enable, like removing zeroing when initialising an array.

Maybe if "treat function calls as possible branch point" is a function-level annotation in LLVM the only difference would be emitting/not emitting that.

[Vurich]

I support enabling this if and only if panic = "abort". Even if having the codegen be different depending on panic mode is not currently supported it would be something deserving of an RFC in my opinion, since there are a bunch of possible optimisations that this could enable, like removing zeroing when initialising an array.

Maybe if "treat function calls as possible branch point" is a function-level annotation in LLVM the only difference would be emitting/not emitting that.

[jrmuizel]

Is it possible for us to get a cargobomb run with the patch reverted to see if we trigger any bad codegen?

[jrmuizel]

Is it possible for us to get a cargobomb run with the patch reverted to see if we trigger any bad codegen?

[dpc]

Any best guesses when this improvement would be available in Rust nightly (with and without panic = "abort")? It would affect the design and implementation of performance critical crates I'm working on. If it's couple of months, it might be worth waiting. If it's a year, it might be worth to workaround in the code.

[dpc]

Any best guesses when this improvement would be available in Rust nightly (with and without panic = "abort")? It would affect the design and implementation of performance critical crates I'm working on. If it's couple of months, it might be worth waiting. If it's a year, it might be worth to workaround in the code.

[arielb1]

@eefriedman

Is PR27860 actually a problem for Rust? I think every time we emit noalias we also emit dereferencable. Unless LLVM considers every GEP of the pointer as dereferencable, even in the presence of e.g. bounds checks (oops).

[arielb1]

@eefriedman

Is PR27860 actually a problem for Rust? I think every time we emit noalias we also emit dereferencable. Unless LLVM considers every GEP of the pointer as dereferencable, even in the presence of e.g. bounds checks (oops).

[arielb1]

PR27860 is actually a problem even with &-references - this segfaults:

#![feature(test)]

extern crate test;

extern "C" {
    fn abort();
}

#[no_mangle]
#[inline(never)]
pub fn abuseme(x: &[u8]) -> u8 {
    if x.len() > 0x10000000000 {
        test::black_box(x[0x10000000000]);
    }
    unsafe { abort(); }
    unsafe { *x.get_unchecked(0x10000000000) }
}

fn main() {
    abuseme(&[]);
}

[arielb1]

PR27860 is actually a problem even with &-references - this segfaults:

#![feature(test)]

extern crate test;

extern "C" {
    fn abort();
}

#[no_mangle]
#[inline(never)]
pub fn abuseme(x: &[u8]) -> u8 {
    if x.len() > 0x10000000000 {
        test::black_box(x[0x10000000000]);
    }
    unsafe { abort(); }
    unsafe { *x.get_unchecked(0x10000000000) }
}

fn main() {
    abuseme(&[]);
}

[Vurich]

@arielb1 Does that violate Rust's guarantees though? I don't know that there's any guarantees about reordering when it comes to calling extern functions. What happens if you explicitly annotate the type as fn abort() -> !?

[Vurich]

@arielb1 Does that violate Rust's guarantees though? I don't know that there's any guarantees about reordering when it comes to calling extern functions. What happens if you explicitly annotate the type as fn abort() -> !?

[arielb1]

@Vurich

Of course there are guarantees about reordering about extern functions - otherwise read etc. would never work. You could also replace the abort with a read of a forever-empty pipe or something to avoid it needing to be annotated with !.

[arielb1]

@Vurich

Of course there are guarantees about reordering about extern functions - otherwise read etc. would never work. You could also replace the abort with a read of a forever-empty pipe or something to avoid it needing to be annotated with !.

[oyvindln]

I support enabling this if and only if panic = "abort".

Maybe a -Z enable-noalias or similar option could be added as a start to make it easier to test where the lack of noalias makes a difference.

[oyvindln]

I support enabling this if and only if panic = "abort".

Maybe a -Z enable-noalias or similar option could be added as a start to make it easier to test where the lack of noalias makes a difference.

[matthiaskrgr]

@oyvindln if you want to turn on newgvn to compare things, RUSTFLAGS="-C llvm-args=-enable-newgvn" might work.

[matthiaskrgr]

@oyvindln if you want to turn on newgvn to compare things, RUSTFLAGS="-C llvm-args=-enable-newgvn" might work.

[pcwalton]

It's unclear from reading this what the state of this bug is. Do we still believe that NewGVN will fix this issue?

[pcwalton]

It's unclear from reading this what the state of this bug is. Do we still believe that NewGVN will fix this issue?

[arielb1]

@pcwalton

I think all "noalias-exclusive" bugs are fixed, so it might be worth giving it a try and opening a PR.

[arielb1]

@pcwalton

I think all "noalias-exclusive" bugs are fixed, so it might be worth giving it a try and opening a PR.

[cuviper]

Does that enablement need to be conditional on LLVM version?

[cuviper]

Does that enablement need to be conditional on LLVM version?

[arielb1]

I think. 4.0 should be fine.

[arielb1]

I think. 4.0 should be fine.

[bstrie]

Update: there's now a -Z flag for opting-in to this on an experimental basis: #45012. It also seems as though the behavior is enabled by default for panic=abort builds. However, I think this might be worth keeping open until we concretely determine whether or not we'll be able to turn this on for panic=unwind builds (see #45029 ).

[bstrie]

Update: there's now a -Z flag for opting-in to this on an experimental basis: #45012. It also seems as though the behavior is enabled by default for panic=abort builds. However, I think this might be worth keeping open until we concretely determine whether or not we'll be able to turn this on for panic=unwind builds (see #45029 ).

[alexbool]

What's the state of this given that LLVM has been upgraded to 6.0?

[alexbool]

What's the state of this given that LLVM has been upgraded to 6.0?

[scottmcm]

To help finding this issue: the -Z flag is mutable_noalias.

[scottmcm]

To help finding this issue: the -Z flag is mutable_noalias.

[scottmcm]

Looks like the last part of the checklist above landed with https://reviews.llvm.org/D38619#937247

[scottmcm]

Looks like the last part of the checklist above landed with https://reviews.llvm.org/D38619#937247

[nox]

Should we make a PR to always enable mutable_noalias and see what happens on the CI and Crater?

Cc @rust-lang/wg-codegen

[nox]

Should we make a PR to always enable mutable_noalias and see what happens on the CI and Crater?

Cc @rust-lang/wg-codegen

[nikic]

As #50744 has landed, this can be closed.

[nikic]

As #50744 has landed, this can be closed.

[alexcrichton]

🎉

[alexcrichton]

🎉