Skip to content
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

Inline ptr::null(_mut) even in debug builds #64996

Merged
merged 1 commit into from
Oct 20, 2019
Merged

Inline ptr::null(_mut) even in debug builds #64996

merged 1 commit into from
Oct 20, 2019

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Oct 2, 2019 •
edited
Loading

I think we should treat ptr::null(_mut) as a constant. As It may help reduce code size
in debug build.
See godbolt link: https://godbolt.org/z/b9YMtD

@rust-highfive
Copy link
Collaborator

r? @Kimundi

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2019
@tesuji
Copy link
Contributor Author

tesuji commented Oct 2, 2019

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned Kimundi Oct 2, 2019
@tesuji tesuji closed this Oct 2, 2019
@tesuji tesuji reopened this Oct 2, 2019
@eddyb
Copy link
Member

eddyb commented Oct 2, 2019

LGTM but I'm not sure what our inline policy is, cc @nagisa @rkruppe @gnzlbg

@nagisa
Copy link
Member

nagisa commented Oct 2, 2019

I am amused that we do not "just" evaluate ptr::null, as it is a const fn with no arguments…

@nagisa
Copy link
Member

nagisa commented Oct 2, 2019

IIRC if there is code that uses #[inline(always)] as opposed to #[inline] the backend automatically toggles opt-level=1, which is what ends up actually making the inline happen. No strong opposition against this change either way.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Oct 2, 2019 •
edited
Loading

IIRC if there is code that uses #[inline(always)] as opposed to #[inline] the backend automatically toggles opt-level=1, which is what ends up actually making the inline happen. No strong opposition against this change either way.

That would surprise me a lot. I know for sure though that we run the AlwaysInline pass at -O0. This explains why the inlining happens and also why there is a pointless jump in the godbolt output (at -O1 that is cleaned up, presumably by SimplifyCFG).

@nagisa
Copy link
Member

nagisa commented Oct 2, 2019

Eeh, my memory failed me then.

@tesuji tesuji mentioned this pull request Oct 2, 2019
Merged
@nagisa
Copy link
Member

nagisa commented Oct 2, 2019

@oli-obk any ideas why const fns like these aren’t evaluated in-line? Should we do that?

@eddyb
Copy link
Member

eddyb commented Oct 3, 2019

I think this would fit nicely into my recent suggestion to treat no-argument const fn as an opaque (i.e. we won't peek at the body) const.
(That was in the context of intrinsics like size_of but this seems similar)

Also, does the constant folding MIR pass handle const fn calls at all today?

@tesuji
Copy link
Contributor Author

tesuji commented Oct 6, 2019

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned eddyb Oct 6, 2019
@tesuji tesuji mentioned this pull request Oct 10, 2019
Merged
@JohnCSimon
Copy link
Member

Ping from triage
@oli-obk can you please review this pr?
cc: @nagisa @rkruppe @gnzlbg

thanks.

tmandry added a commit to tmandry/rust that referenced this pull request Oct 18, 2019
@tmandry
Rollup merge of rust-lang#65016 - lzutao:inline-mem-constfn, r=oli-obk
13e47c8 
Always inline `mem::{size_of,align_of}` in debug builds

Those two are const fn and do not have any arguments. Inlining
helps reducing generated code size in debug builds.

See also rust-lang#64996.
tmandry added a commit to tmandry/rust that referenced this pull request Oct 18, 2019
@tmandry
Rollup merge of rust-lang#65016 - lzutao:inline-mem-constfn, r=oli-obk
7167a59 
Always inline `mem::{size_of,align_of}` in debug builds

Those two are const fn and do not have any arguments. Inlining
helps reducing generated code size in debug builds.

See also rust-lang#64996.
@oli-obk
Copy link
Contributor

oli-obk commented Oct 19, 2019

@bors r+ rollup

Let's do this until const prop can handle function calls

@bors
Copy link
Contributor

bors commented Oct 19, 2019

📌 Commit d0862ec has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 19, 2019
@Centril
Rollup merge of rust-lang#64996 - lzutao:inline-ptr-null, r=oli-obk
4d96185 
Inline `ptr::null(_mut)` even in debug builds

I think we should treat `ptr::null(_mut)` as a constant. As It may help reduce code size
in debug build.
See godbolt link: https://godbolt.org/z/b9YMtD
@Centril Centril mentioned this pull request Oct 19, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Oct 20, 2019
@Centril
Rollup merge of rust-lang#64996 - lzutao:inline-ptr-null, r=oli-obk
f01ffbc 
Inline `ptr::null(_mut)` even in debug builds

I think we should treat `ptr::null(_mut)` as a constant. As It may help reduce code size
in debug build.
See godbolt link: https://godbolt.org/z/b9YMtD
@Centril Centril mentioned this pull request Oct 20, 2019
Merged
bors added a commit that referenced this pull request Oct 20, 2019
@bors
Auto merge of #65622 - Centril:rollup-l8orba7, r=Centril
89e645a 
Rollup of 6 pull requests

Successful merges:

 - #64996 (Inline `ptr::null(_mut)` even in debug builds)
 - #65551 (Avoid realloc in `CString::new`)
 - #65593 (add test for calling non-const fn)
 - #65595 (move `parse_cfgspecs` to `rustc_interface`)
 - #65600 (Remove unneeded `ref` from docs)
 - #65602 (Fix plural mistake in emitter.rs)

Failed merges:

r? @ghost
@bors bors merged commit d0862ec into rust-lang:master Oct 20, 2019
@tesuji tesuji deleted the inline-ptr-null branch October 20, 2019 18:10
@therealprof therealprof mentioned this pull request Oct 29, 2019
Merged
@sinkuu sinkuu mentioned this pull request Oct 30, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@tesuji @rust-highfive @eddyb @nagisa @hanna-kruppe @JohnCSimon @oli-obk @bors @Kimundi