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

[x86] Consider mov-to-push also at -O2? #26699

Closed
zmodem opened this issue Jan 26, 2016 · 3 comments
Closed

[x86] Consider mov-to-push also at -O2? #26699

zmodem opened this issue Jan 26, 2016 · 3 comments
Labels
backend:X86 bugzilla Issues migrated from bugzilla

Comments

@zmodem
Copy link
Collaborator

zmodem commented Jan 26, 2016

Bugzilla Link 26325
Resolution FIXED
Resolved on Nov 03, 2016 19:23
Version trunk
OS Linux
Blocks #26673
CC @emaste,@rotateright

Extended Description

On Windows, most of Chromium builds with /O1 /Os (optimize for size), but some of it builds with /O2 /Ot (optimize for speed).

For this code:

void f(int x, int y);
void g(int x) {
f(x, 1);
}

clang -target i686-pc-win32 -Os generates:

00000000 <?g@@Yaxh@Z>:
0: 6a 01 push $0x1
2: ff 74 24 08 pushl 0x8(%esp)
6: e8 00 00 00 00 call b <?g@@Yaxh@Z+0xb>
b: 83 c4 08 add $0x8,%esp
e: c3 ret

but with -O2 it generates:

00000000 <?g@@Yaxh@Z>:
0: 83 ec 08 sub $0x8,%esp
3: 8b 44 24 0c mov 0xc(%esp),%eax
7: 89 04 24 mov %eax,(%esp)
a: c7 44 24 04 01 00 00 movl $0x1,0x4(%esp)
11: 00
12: e8 00 00 00 00 call 17 <?g@@Yaxh@Z+0x17>
17: 83 c4 08 add $0x8,%esp
1a: c3 ret

Is the -O2 version actually faster enough to justify the size increase here?

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 27, 2016

I tend to agree.

I originally wanted it for all opt levels, but there was some pushback because of potential performance concerns. And since my main concern was -Os, I just never got to benchmarking it for -O2.

Adding Dave, who's probably a more appropriate contact for this than I am right now.

@zmodem
Copy link
Collaborator Author

zmodem commented Mar 31, 2016

r264966

@EdSchouten
Copy link
Contributor

mentioned in issue llvm/llvm-bugzilla-archive#30879

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

3 participants