Page MenuHomePhabricator

[lld] Support size levels with (Thin)LTO
AbandonedPublic

Authored by phosek on May 12 2020, 3:42 PM.

Details

Summary

Currently the size levels aren't supported at all and attempt to use
these with (Thin)LTO results in failure because lld doesn't support
anything other than optimization levels. This change extends the option
handling to properly handle the size level and pass it to the LTO
backend.

Diff Detail

Event Timeline

phosek created this revision.May 12 2020, 3:42 PM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
ychen added subscribers: pcc, ychen.May 12 2020, 4:29 PM

FYI D72404. @pcc 's opinion is this is changing direction.

Thanks for linking my previous comment from last summer, I don't understand why we thread this through while it is intended to be carried by function attributes?

MaskRay added a comment.EditedMay 13 2020, 2:51 PM

@mehdi_amini @pcc @tejohnson

I just saw another issue saying that clang -Os -flto=thin a.c does not work https://bugs.llvm.org/show_bug.cgi?id=45913 (+ @manojgupta)
I marked it as yet another duplicate of PR42445

For -Os, clang driver passes -plugin-opt=Os (https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChains/CommonArgs.cpp#L402) but neither LLVMgold.so nor LLD recognizes -plugin-opt=Os.
-Oz is similar. What is the concrete suggestion here? For quick reference, the -plugin-opt=O logic was added by rL256146.

@mehdi_amini @pcc @tejohnson

I just saw another issue saying that clang -Os -flto=thin a.c does not work https://bugs.llvm.org/show_bug.cgi?id=45913 (+ @manojgupta)
I marked it as yet another duplicate of PR42445

For -Os, clang driver passes -plugin-opt=Os (https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChains/CommonArgs.cpp#L402) but neither LLVMgold.so nor LLD recognizes -plugin-opt=Os.
-Oz is similar. What is the concrete suggestion here? For quick reference, the -plugin-opt=O logic was added by rL256146.

The issue form my perspective is the inconsistency. Today, lld accepts -plugin-opt=O<number> but it doesn't accept -plugin-opt=O<letter>, but Clang automatically translates -O<anything> to -plugin-opt=O<anything> which results in a failure when your try to use -Os, -Oz or -Og with (Thin)LTO.

If the official stance of LTO maintainers is that we should be relying on function attributes and avoid overriding the optimization level via flags, then I'd argue that we should remove -plugin-opt=O<number> from lld altogether. The partial support just leads to confusion as demonstrated by some of the issues others have already referenced. In fact this change was motivated by several internal bugs from developers who tried to use ThinLTO with -Oz and hit the error even though -O2 worked fine.

pcc added a comment.May 13 2020, 4:47 PM

@mehdi_amini @pcc @tejohnson

I just saw another issue saying that clang -Os -flto=thin a.c does not work https://bugs.llvm.org/show_bug.cgi?id=45913 (+ @manojgupta)
I marked it as yet another duplicate of PR42445

For -Os, clang driver passes -plugin-opt=Os (https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChains/CommonArgs.cpp#L402) but neither LLVMgold.so nor LLD recognizes -plugin-opt=Os.
-Oz is similar. What is the concrete suggestion here? For quick reference, the -plugin-opt=O logic was added by rL256146.

The issue form my perspective is the inconsistency. Today, lld accepts -plugin-opt=O<number> but it doesn't accept -plugin-opt=O<letter>, but Clang automatically translates -O<anything> to -plugin-opt=O<anything> which results in a failure when your try to use -Os, -Oz or -Og with (Thin)LTO.

This is where the bug is then. The clang driver should be translating these arguments to -plugin-opt O2.

MaskRay added a comment.EditedMay 13 2020, 5:19 PM
In D79818#2035448, @pcc wrote:

@mehdi_amini @pcc @tejohnson

I just saw another issue saying that clang -Os -flto=thin a.c does not work https://bugs.llvm.org/show_bug.cgi?id=45913 (+ @manojgupta)
I marked it as yet another duplicate of PR42445

For -Os, clang driver passes -plugin-opt=Os (https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChains/CommonArgs.cpp#L402) but neither LLVMgold.so nor LLD recognizes -plugin-opt=Os.
-Oz is similar. What is the concrete suggestion here? For quick reference, the -plugin-opt=O logic was added by rL256146.

The issue form my perspective is the inconsistency. Today, lld accepts -plugin-opt=O<number> but it doesn't accept -plugin-opt=O<letter>, but Clang automatically translates -O<anything> to -plugin-opt=O<anything> which results in a failure when your try to use -Os, -Oz or -Og with (Thin)LTO.

This is where the bug is then. The clang driver should be translating these arguments to -plugin-opt O2.

Thanks. Preparing a patch to address -Os -Oz -Og -O.

edit: Created D79919

@mehdi_amini @pcc @tejohnson

I just saw another issue saying that clang -Os -flto=thin a.c does not work https://bugs.llvm.org/show_bug.cgi?id=45913 (+ @manojgupta)
I marked it as yet another duplicate of PR42445

For -Os, clang driver passes -plugin-opt=Os (https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChains/CommonArgs.cpp#L402) but neither LLVMgold.so nor LLD recognizes -plugin-opt=Os.
-Oz is similar. What is the concrete suggestion here? For quick reference, the -plugin-opt=O logic was added by rL256146.

The issue form my perspective is the inconsistency. Today, lld accepts -plugin-opt=O<number> but it doesn't accept -plugin-opt=O<letter>, but Clang automatically translates -O<anything> to -plugin-opt=O<anything> which results in a failure when your try to use -Os, -Oz or -Og with (Thin)LTO.

If the official stance of LTO maintainers is that we should be relying on function attributes and avoid overriding the optimization level via flags, then I'd argue that we should remove -plugin-opt=O<number> from lld altogether. The partial support just leads to confusion as demonstrated by some of the issues others have already referenced. In fact this change was motivated by several internal bugs from developers who tried to use ThinLTO with -Oz and hit the error even though -O2 worked fine.

The underlying issue is that O<number> vs O<letter> are somehow-but-not-really orthogonal options. Internally also, O<number> impacts the pass pipeline which isn't setup to be handled per-function, while the O<letter> is mostly communicated by adding a function attribute and does not fundamentally need to impact the optimizer pass pipeline.
Adding LTO into the mix makes it harder to reconcile: should we override the compile-time function attribute based on the link-time flag? On the other hand O<number> does not have the same problem with LTO.

@mehdi_amini @pcc @tejohnson

I just saw another issue saying that clang -Os -flto=thin a.c does not work https://bugs.llvm.org/show_bug.cgi?id=45913 (+ @manojgupta)
I marked it as yet another duplicate of PR42445

For -Os, clang driver passes -plugin-opt=Os (https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChains/CommonArgs.cpp#L402) but neither LLVMgold.so nor LLD recognizes -plugin-opt=Os.
-Oz is similar. What is the concrete suggestion here? For quick reference, the -plugin-opt=O logic was added by rL256146.

The issue form my perspective is the inconsistency. Today, lld accepts -plugin-opt=O<number> but it doesn't accept -plugin-opt=O<letter>, but Clang automatically translates -O<anything> to -plugin-opt=O<anything> which results in a failure when your try to use -Os, -Oz or -Og with (Thin)LTO.

If the official stance of LTO maintainers is that we should be relying on function attributes and avoid overriding the optimization level via flags, then I'd argue that we should remove -plugin-opt=O<number> from lld altogether. The partial support just leads to confusion as demonstrated by some of the issues others have already referenced. In fact this change was motivated by several internal bugs from developers who tried to use ThinLTO with -Oz and hit the error even though -O2 worked fine.

The underlying issue is that O<number> vs O<letter> are somehow-but-not-really orthogonal options. Internally also, O<number> impacts the pass pipeline which isn't setup to be handled per-function, while the O<letter> is mostly communicated by adding a function attribute and does not fundamentally need to impact the optimizer pass pipeline.
Adding LTO into the mix makes it harder to reconcile: should we override the compile-time function attribute based on the link-time flag? On the other hand O<number> does not have the same problem with LTO.

I see that -O<number> can affect regular LTO/ThinLTO pass manager passes (populate*PassManager (legacy PM) and build*Pipeline (new PM) called by LTOBackend.cpp:opt). Mapping -Os and -Oz to SpeedLevel 2 may be the most user convenient thing.

phosek abandoned this revision.May 14 2020, 1:34 PM

No longer needed after D79919 landed.