Skip to content

decide whether global addresses should be significant by default #8958

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.

Sign up for GitHub

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

Jump to bottom
Closed
thestinger opened this issue Sep 3, 2013 · 14 comments · Fixed by #14977
Closed

decide whether global addresses should be significant by default #8958

thestinger opened this issue Sep 3, 2013 · 14 comments · Fixed by #14977
Labels
I-slow Issue: Problems and improvements with respect to performance of generated code.
Milestone
1.0

Comments

@thestinger
Copy link
Contributor

We have #[address_insignificant] now because it's needed to have LLVM merge constants. I think insignificant addresses should be the default, and TLS should be done with a macro applying an #[address_significant] attribute.

Issue #8957 covers functions, since the situation is different as we already merge them.
@thestinger
Copy link
Contributor Author

Nominating for the backwards compatible milestone (well defined would work too).

@alexcrichton
Copy link
Member

If we do decide to do this, an interesting statistic to see beforehand would be the size difference in the standard libraries or librustc.

When I was dealing with address_insignificant I had to flag all of the statics as internal and private linkage, so this might not be applicable to pub statics, but that too would need further investigation to see what happens.

@thestinger
Copy link
Contributor Author

One thing to note is that string constants are already merged, so only the address of the slice object itself is significant right now.

@brson
Copy link
Contributor

brson commented Sep 5, 2013

TLS should not involve fn addresses at all - it was an expedient. Agree we shouldn't make promises about fn addresses if it's useful for optimizations.

@catamorphism
Copy link
Contributor

Accepted backwards-compatible

@nikomatsakis
Copy link
Contributor

does this relate to static muts?

@thestinger
Copy link
Contributor Author

I thought this was only doable for constants, but it does appear you could tag static mut with unnamed_addr and it wouldn't merge them unless it can prove the content differences between the two are insignificant too.

I don't think it would ever actually result in an optimization with static mut, since a single write would make the significant of the content significance prevent merging.

@brson
Copy link
Contributor

brson commented Oct 31, 2013

I think addresses should be insignifacant and tls should be done with type_id.

@alexcrichton
Copy link
Member

Nominating for closure with the following policy:

  • All static mut variables have significant addresses
  • All static variables have insignificant addresses unless tagged with #[significant_address]

@brson
Copy link
Contributor

brson commented May 8, 2014

Maybe #[inline(never)] instead of #[significant_adress].

@brson brson removed the I-nominated label May 8, 2014
@brson
Copy link
Contributor

brson commented May 8, 2014

Sounds like there's some more to do here. Leaving open.

@brson
Copy link
Contributor

brson commented Jun 9, 2014

What's the current thinking here?

  • All static mut are significant
  • All static are insignificant unless they contain certain types (which?) or have #[inline(never)]?

@brson brson mentioned this issue Jun 9, 2014
Closed
@alexcrichton
Copy link
Member

@brson, that is my understanding.

@pcwalton
Copy link
Contributor

I am working on this, but it's hit some issues with the way we handle inlining with #[address_insignificant]. I believe what we were trying to do before was pretty bogus. I'm now attempting to work around it.

pcwalton added a commit to pcwalton/rust that referenced this issue Jun 17, 2014
@pcwalton
librustc: Make addresses of immutable statics insignificant unless
cad760b 
`#[inline(never)]` is used.

Closes rust-lang#8958.

This can break some code that relied on the addresses of statics
being distinct; add `#[inline(never)]` to the affected statics.

[breaking-change]
@pcwalton pcwalton mentioned this issue Jun 17, 2014
Merged
bors added a commit that referenced this issue Jun 17, 2014
@bors
auto merge of #14977 : pcwalton/rust/address-insignificant-reform, r=…
ed33080 
…brson

`#[inline(never)]` is used.

Closes #8958.

This can break some code that relied on the addresses of statics
being distinct; add `#[inline(never)]` to the affected statics.

[breaking-change]

r? @brson
flip1995 pushed a commit to flip1995/rust that referenced this issue Jul 18, 2022
@bors
Auto merge of rust-lang#8958 - Alexendoo:simple_filter_map, r=giraffate
54feac1 
Lint simple expressions in `manual_filter_map`, `manual_find_map`

changelog: Lint simple expressions in [`manual_filter_map`], [`manual_find_map`]

The current comparison rules out `.find(|a| a.is_some()).map(|b| b.unwrap())` because `a` being a reference can effect more complicated expressions, this adds a simple check for that case and adds the necessary derefs

There's some overlap with `option_filter_map` so `lint_filter_some_map_unwrap` now returns a `bool` to indicate it linted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Milestone
1.0
Development

Successfully merging a pull request may close this issue.

librustc: Make addresses of immutable statics insignificant unless pcwalton/rust
6 participants
@alexcrichton @brson @nikomatsakis @pcwalton @catamorphism @thestinger