/rust

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround LLVM optimizer bug by not marking &mut pointers as noalias #31545

Merged
merged 1 commit into from Feb 12, 2016

Conversation

Reviewers
No reviews
Assignees

@jroesch jroesch

Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@dotdash @rust-highfive @alexcrichton @bors @ticki @jroesch
@dotdash
Contributor

dotdash commented Feb 10, 2016

LLVM's memory dependence analysis doesn't properly account for calls
that could unwind and thus effectively act as a branching point. This
can lead to stores that are only visible when the call unwinds being
removed, possibly leading to calls to drop() functions with b0rked
memory contents.

As there is no fix for this in LLVM yet and we want to keep
compatibility to current LLVM versions anyways, we have to workaround
this bug by omitting the noalias attribute on &mut function arguments.
Benchmarks suggest that the performance loss by this change is very
small.

Thanks to @RalfJung for pushing me towards not removing too many
noalias annotations and @alexcrichton for helping out with the test for
this bug.

Fixes #29485

@rust-highfive rust-highfive assigned jroesch Feb 10, 2016

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Feb 10, 2016

Collaborator

r? @jroesch

(rust_highfive has picked a reviewer for you, use r? to override)
Collaborator

rust-highfive commented Feb 10, 2016

r? @jroesch

(rust_highfive has picked a reviewer for you, use r? to override)
src/librustc_trans/trans/attributes.rs
@@ -265,7 +265,7 @@ pub fn from_fn_type<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, fn_type: ty::Ty<'tcx
// on memory dependencies rather than pointer equality
let interior_unsafe = mt.ty.type_contents(ccx.tcx()).interior_unsafe();
if mt.mutbl == hir::MutMutable || !interior_unsafe {
if mt.mutbl != hir::MutMutable && !interior_unsafe {

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 10, 2016

Member

I may not quite be understanding what's going on here, but doesn't this mean that &usize will have the noalias attribute applied?
Copy URL
@alexcrichton

alexcrichton Feb 10, 2016

Member

I may not quite be understanding what's going on here, but doesn't this mean that &usize will have the noalias attribute applied?

This comment has been minimized.

Show comment
Hide comment
@dotdash

dotdash Feb 10, 2016

Contributor

Yes, and that's intentional. The change is supposed to only affect &mut references (and '&' ones with unsafe interiors), because those are the ones that could have stores that would expose this bug.
Copy URL
@dotdash

dotdash Feb 10, 2016

Contributor

Yes, and that's intentional. The change is supposed to only affect &mut references (and '&' ones with unsafe interiors), because those are the ones that could have stores that would expose this bug.

This comment has been minimized.

Show comment
Hide comment
@dotdash

dotdash Feb 10, 2016

Contributor

Oh, also note that WRT &usize this is not a change, those pointers got the noalias attribute before as well (the || got changed into a &&). As the comment above this change says, noalias is about memory dependencies rather than plain pointer equality.
Copy URL
@dotdash

dotdash Feb 10, 2016

Contributor

Oh, also note that WRT &usize this is not a change, those pointers got the noalias attribute before as well (the || got changed into a &&). As the comment above this change says, noalias is about memory dependencies rather than plain pointer equality.

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 10, 2016

Member

Oh, right!
Copy URL
@alexcrichton

alexcrichton Feb 10, 2016

Member

Oh, right!
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 10, 2016

Member

Looks like the travis failure is legit in that a codegen test needs to be udpated.
Member

alexcrichton commented Feb 10, 2016

Looks like the travis failure is legit in that a codegen test needs to be udpated.
@dotdash
Workaround LLVM optimizer bug by not marking &mut pointers as noalias 
LLVM's memory dependence analysis doesn't properly account for calls
that could unwind and thus effectively act as a branching point. This
can lead to stores that are only visible when the call unwinds being
removed, possibly leading to calls to drop() functions with b0rked
memory contents.

As there is no fix for this in LLVM yet and we want to keep
compatibility to current LLVM versions anyways, we have to workaround
this bug by omitting the noalias attribute on &mut function arguments.
Benchmarks suggest that the performance loss by this change is very
small.

Thanks to @RalfJung for pushing me towards not removing too many
noalias annotations and @alexcrichton for helping out with the test for
this bug.

Fixes #29485
Loading status checks…
a17fb64
@dotdash

This comment has been minimized.

Show comment
Hide comment
@dotdash

dotdash Feb 10, 2016

Contributor

Updated the codegen test, completely forgot about those and only ran the new test before opening the PR
Contributor

dotdash commented Feb 10, 2016

Updated the codegen test, completely forgot about those and only ran the new test before opening the PR
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 10, 2016

Member

@bors: r+ a17fb64
Member

alexcrichton commented Feb 10, 2016

@bors: r+ a17fb64
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Feb 11, 2016

Contributor

⌛️ Testing commit a17fb64 with merge 98ec51a...
Contributor

bors commented Feb 11, 2016

⌛️ Testing commit a17fb64 with merge 98ec51a...

bors added a commit that referenced this pull request Feb 11, 2016

@bors
Auto merge of #31545 - dotdash:no_noalias, r=alexcrichton 
LLVM's memory dependence analysis doesn't properly account for calls
that could unwind and thus effectively act as a branching point. This
can lead to stores that are only visible when the call unwinds being
removed, possibly leading to calls to drop() functions with b0rked
memory contents.

As there is no fix for this in LLVM yet and we want to keep
compatibility to current LLVM versions anyways, we have to workaround
this bug by omitting the noalias attribute on &mut function arguments.
Benchmarks suggest that the performance loss by this change is very
small.

Thanks to @RalfJung for pushing me towards not removing too many
noalias annotations and @alexcrichton for helping out with the test for
this bug.

Fixes #29485
98ec51a
@bors

@bors bors merged commit a17fb64 into rust-lang:master Feb 12, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@llogiq llogiq referenced this pull request Feb 14, 2016

Closed

TWIR 118's COTW + interesting PRs #170

@ticki

This comment has been minimized.

Show comment
Hide comment
@ticki

ticki Feb 15, 2016

Contributor

This seems like a horrible performance loss. Can you post the benchmarks?
Contributor

ticki commented Feb 15, 2016

This seems like a horrible performance loss. Can you post the benchmarks?
@dotdash

This comment has been minimized.

Show comment
Hide comment
@dotdash

dotdash Feb 15, 2016

Contributor

@ticki the results of the benchmarks we made are in #29485 -- do you have any results that show that "horrible performance loss"?
Contributor

dotdash commented Feb 15, 2016

@ticki the results of the benchmarks we made are in #29485 -- do you have any results that show that "horrible performance loss"?
@ticki

This comment has been minimized.

Show comment
Hide comment
@ticki

ticki Feb 15, 2016

Contributor

Well, I haven't benchmarked this change, but in my experience, disabling noalias in GCC gave bad performance.
Contributor

ticki commented Feb 15, 2016

Well, I haven't benchmarked this change, but in my experience, disabling noalias in GCC gave bad performance.

@bstrie bstrie referenced this pull request Feb 15, 2016

Closed

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

@dotdash

This comment has been minimized.

Show comment
Hide comment
@dotdash

dotdash Feb 15, 2016

Contributor

@ticki what exactly do you mean by "disabling noalias in GCC"? Making it ignore the restrict keyword? Because when comparing to C, that's basically the only change we made here.
Contributor

dotdash commented Feb 15, 2016

@ticki what exactly do you mean by "disabling noalias in GCC"? Making it ignore the restrict keyword? Because when comparing to C, that's basically the only change we made here.
@ticki

This comment has been minimized.

Show comment
Hide comment
@ticki

ticki Feb 15, 2016

Contributor

-fno-strict-aliasing, that is.
Contributor

ticki commented Feb 15, 2016

-fno-strict-aliasing, that is.
@dotdash

This comment has been minimized.

Show comment
Hide comment
@dotdash

dotdash Feb 15, 2016

Contributor

That option disables TBAA, which rust does not use at all. The scope of
this change is smaller than that of that option.

-fno-strict-aliasing, that is.

—
Reply to this email directly or view it on GitHub
#31545 (comment).
Contributor

dotdash commented Feb 15, 2016

That option disables TBAA, which rust does not use at all. The scope of
this change is smaller than that of that option.

-fno-strict-aliasing, that is.

—
Reply to this email directly or view it on GitHub
#31545 (comment).

@bluss bluss referenced this pull request Mar 12, 2016

Closed

Nightly performance issue with vec::extend_with_element #32155

@dotdash dotdash deleted the dotdash:no_noalias branch Mar 18, 2016

@frankmcsherry frankmcsherry referenced this pull request May 9, 2016

Closed

Performance issue in `write_all` (`Vec::extend_from_slice`) #33518

@crumblingstatue crumblingstatue referenced this pull request Mar 10, 2017

Open

n-body performance degradation from 1.7 to 1.8 #35

@jrmuizel jrmuizel referenced this pull request May 1, 2017

Open

Arithmetic not folded #41631

@jrmuizel jrmuizel referenced this pull request Jul 19, 2017

Open

Result-based error handling optimizes very poorly for large payloads #42047

@Gankro Gankro referenced this pull request Oct 4, 2017

Open

mutable noalias: re-enable permanently, only for panic=abort, or stabilize flag? #45029

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment