search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.34k stars 12.13k forks source link

le32-unknown-nacl target does not optimize simple struct return values as primitive returns #19237

Open llvmbot opened 10 years ago

llvmbot commented 10 years ago
Bugzilla Link 18863
Version 3.4
OS All
Blocks llvm/llvm-project#15837
Attachments sret.cpp, sret-amd64.ll, sret-le32.ll
Reporter LLVM Bugzilla Contributor
CC @kripken,@dschuff,@jfbastien,@sunfishcode

Extended Description

The attached sret.cpp demonstrates a simple struct with a nondefault constructor.

When compiled to IR for the default x86_64 target on Ubuntu 12.04 AMD64, it optimizes the S struct into a simple i32. See the attached sret-amd64.ll.

When compiled to IR for the le32-unknown-nacl target, the struct is not optimized away. See the attached sret-le32.ll.

Both files were generated with:

clang --std=c++11 -O2 -emit-llvm -target le32-unknown-nacl -c -Wall -o sret.bc sret.cpp && llvm-dis -o sret-le32.ll sret.bc

clang --std=c++11 -O2 -emit-llvm -c -Wall -o sret.bc sret.cpp && llvm-dis -o sret-amd64.ll sret.bc

This reproduces with both clang 3.3 and clang 3.4.

The lack of this optimization penalizes use of utility types, such as InternedString and IntrusiveSmartPtr, used frequently in our Emscripten-compiled project.

jfbastien commented 10 years ago

As a reference, Dan's asmjs-unknown-emscripten patch: https://github.com/kripken/emscripten-fastcomp-clang/commit/f6e2fddff6294c1c2ec2f9602c30ff680359ed19

jfbastien commented 10 years ago

I think this is part of the le32-- behavior, and talking to jvoung/dschuff it sounds like we initially wanted to allow struct returns in PNaCl but then removed struct types to simplify things, so in PNaCl structs with a single integer get transformed to a single integers in a later pass. In NaCl there is no ABI issue (the IRT is the only external consumer of the ABI adn we build it ourselves). Same stands for Emscripten.

It therefore seems like changing Clang to emit int returns when returning a struct with a single integer in le32-- mode is acceptable.

kripken commented 10 years ago

Ok, then I guess we should fix this in the new emscripten backend (which will then hopefully get upstreamed eventually when the backend is ready together with everything else).

I think we can close this and focus on

https://github.com/kripken/emscripten/issues/2135

sunfishcode commented 10 years ago

Changing how clang lowers this would break the NaCl ABI, and I don't know what we can do there. It should be pretty easy to fix in the emscripten back end though.