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.

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

Inline `{min,max}_value` even in debug builds #64941

Merged
merged 1 commit into from Oct 3, 2019

Conversation

@lzutao
Copy link
Contributor

commented Oct 1, 2019

I think it is worth to inline {min,max}_value even in debug builds.
See this godbolt link: https://godbolt.org/z/-COkVS

@lzutao lzutao force-pushed the lzutao:inline-max_min_value branch from a7dec48 to 3b49ab6 Oct 1, 2019
@Centril

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

commented Oct 1, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

⌛️ Trying commit 3b49ab6 with merge 6143092...

bors added a commit that referenced this pull request Oct 1, 2019
Experimental: Inline `{min,max}_value` even in debug builds

Hopefully runtime in debug build will faster somehow!
r? @ghost
Could I have a perf run? @Centril
@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

☀️ Try build successful - checks-azure
Build commit: 6143092 (61430923b8b2f3e7565c7aa30a3eb96474641bd6)

@rust-timer

This comment has been minimized.

Copy link

commented Oct 1, 2019

Queued 6143092 with parent 22bc9e1, future comparison URL.

@jonas-schievink

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

Isn't perf only measuring compiler performance (which is always built in release mode)?

@lzutao

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

So how can I measure debug build of compiler?

I saw this code generated from debug build (godbolt link: https://godbolt.org/z/9cpH6U)

core::num::<impl u32>::max_value:
        mov     eax, 4294967295
        ret

example::bar:
        push    rax
        call    core::num::<impl u32>::max_value
        mov     dword ptr [rsp + 4], eax
        mov     eax, dword ptr [rsp + 4]
        pop     rcx
        ret

That code is generated from:

pub fn bar() -> u32 {
    u32::max_value()
}

So I think max_value should always be inlined?

@lzutao lzutao changed the title Experimental: Inline `{min,max}_value` even in debug builds Inline `{min,max}_value` even in debug builds Oct 2, 2019
@lzutao

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2019

r? @nnethercote (randomly assign)

@nnethercote

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

📌 Commit 3b49ab6 has been approved by nnethercote

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

⌛️ Testing commit 3b49ab6 with merge aa84c93...

bors added a commit that referenced this pull request Oct 3, 2019
Inline `{min,max}_value` even in debug builds

I think it is worth to inline `{min,max}_value` even in debug builds.
See this godbolt link: https://godbolt.org/z/-COkVS
Centril added a commit to Centril/rust that referenced this pull request Oct 3, 2019
…hercote

Inline `{min,max}_value` even in debug builds

I think it is worth to inline `{min,max}_value` even in debug builds.
See this godbolt link: https://godbolt.org/z/-COkVS
@Centril Centril referenced this pull request Oct 3, 2019
@Centril

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

@bors retry rolled up.

bors added a commit that referenced this pull request Oct 3, 2019
Rollup of 2 pull requests

Successful merges:

 - #64941 (Inline `{min,max}_value` even in debug builds)
 - #65002 (Update llvm-project submodule)

Failed merges:

r? @ghost
@bors bors merged commit 3b49ab6 into rust-lang:master Oct 3, 2019
4 of 5 checks passed
4 of 5 checks passed
homu Testing commit 3b49ab6e4816fd79cf77d0813ceb17d5696d577c with merge aa84c93d98bab674e7b7953cc30dd70f6d4cd0c1...
Details
pr Build #20191001.12 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

☔️ The latest upstream changes (presumably #65045) made this pull request unmergeable. Please resolve the merge conflicts.

@lzutao lzutao deleted the lzutao:inline-max_min_value branch Oct 3, 2019
@scottmcm

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

The godbolt example says to me to just stop using max_value(), not to add an attribute.

See also rust-lang/rfcs#2700

@lzutao

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

@scottmcm What do you think about that case in #64996 and #65016 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.