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

Add LTO build option #505

Merged
merged 3 commits into from
Jul 12, 2024
Merged

Add LTO build option #505

merged 3 commits into from
Jul 12, 2024

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Jun 4, 2024

An old PR which I used as a base:
#150

An old PR which I used as a base:
WebAssembly#150

Co-Authored-by: Dan Gohman <[email protected]>
# LTO; no, full, or thin
# Note: thin LTO here is just for experimentation. It has known issues:
# - https://github.com/llvm/llvm-project/issues/91700
# - https://github.com/llvm/llvm-project/issues/91711
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to these issue I think you will find that any symbols that implement libcalls, or required by the libcall implemenation must currently be excluded from LTO. See https://github.com/emscripten-core/emscripten/blob/3de1b9091adbadc46706959bd97efa033214bc2c/tools/system_libs.py#L1007-L1020

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it still necessary after https://reviews.llvm.org/D71738? do you have a test case?

Copy link
Contributor Author

@yamt yamt Jun 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have excluded __cxa_atexit since then.
other things listed in your emscripten link seem like compiler-rt, which i don't have a plan to build with LTO for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one issue is that compiler-rt can have transitive dependencies into libc. I imagine you will run into similar issues here once you start testing more extensively with LTO, but perhaps I'm wrong, or perhaps things have changed in some way.

Anyway, I guess its fine to land this and then fix issues as they come up in the field (which is basically what we did in emscripten).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i glanced at compiler-rt.
i couldn't find problematic transitive dependencies.
obvious dependencies like memset etc are covered by https://reviews.llvm.org/D71738.

This fixes a failure in wasi-sdk "make check".
("undefined symbol: __cxa_atexit" for ctors_dtors.c test)
@yamt
Copy link
Contributor Author

yamt commented Jun 7, 2024

wasi-sdk counterpart WebAssembly/wasi-sdk#424

@yamt yamt closed this Jul 5, 2024
@yamt yamt reopened this Jul 5, 2024
@yamt
Copy link
Contributor Author

yamt commented Jul 5, 2024

can this land?

@yamt
Copy link
Contributor Author

yamt commented Jul 5, 2024

closed by mistake and then reopened.

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. We can always add object to LIBC_NONLTO_OBJS later if we find any issues going forward.

Makefile Outdated
@@ -627,6 +658,8 @@ $(SYSROOT_LIB)/libsetjmp.a: $(LIBSETJMP_OBJS)

$(PIC_OBJS): CFLAGS += -fPIC -fvisibility=default

$(LIBC_NONLTO_OBJS): CFLAGS += -fno-lto
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I wonder if we could instead filter out the -flto flag instead of passing both -flto and -fno-lto?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@yamt
Copy link
Contributor Author

yamt commented Jul 12, 2024

wasi-sdk counterpart WebAssembly/wasi-sdk#424

cmake version WebAssembly/wasi-sdk#436

Copy link
Collaborator

@abrown abrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's do it. I haven't tested this but, for future readers, realize that this is still experimental. As @yamt and @sbc100 mention above: we can fix issues with this as they appear.

@abrown abrown merged commit b9e15a8 into WebAssembly:main Jul 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants