-
Notifications
You must be signed in to change notification settings - Fork 3k
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
GCC: Add a build profile extension with the link-time optimizer enabled #11856
GCC: Add a build profile extension with the link-time optimizer enabled #11856
Conversation
Do we want develop profile enabled LTO as well? @fkjagodzinski @bulislaw |
@fkjagodzinski, thank you for your changes. |
5282cd4
to
3d07b88
Compare
Force-pushed a fix for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The notes on this change suggest that the release profile has been changed, but the code changes also affect the debug and develop profiles too.
The move of the '-c' option from common to the asm, c and cxx tools appears to be an unrelated change, as is the corresponding change in gcc.py.
@mark-edgeworth, do you prefer the
Or should I update the commit message to point to lto as the cause of this patch? |
Thanks for the docs reference. Looks like making these flags common is the right way to go. |
3d07b88
to
6d6b3e0
Compare
Updated the last commit message to provide a full quote from GCC man. |
Hi, @fkjagodzinski @ARMmbed/mbed-os-maintainers @bulislaw , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a breaking change, but it can break some users. We should highlight it in release notes and maybe a blog post. Any strong feelings about develop? I think debuggability of lto builds is vastly improved in 8/9 @kjbracey-arm @pan-
@fkjagodzinski Do you know if linker plugin is active on gcc arm ? If not it could be useful to add it to the linker flags. Shouldn't you add |
I've no personal experience of working on LTO builds. The claim is debugging is vastly improved in 8/9. But I'm generally wary of such claims - docs claim I was about to make a point against LTO-for-develop on build time, but your tests show builds as faster with LTO on. Somewhat surprised - I guess the compilation is faster, but the link is slower. So non-clean builds will be slower. Could you check how much slower? Main reason to have it on in develop is it means more likely to catch undefined behaviour traps exposed by removed function-barriers-to-optimisation in LTO. Reduces the urgency of separate release testing. |
Where the option would matter to the linker would be if you wanted to say |
Regarding the I'll get back to you when I check other flags. Thanks for the feedback! |
I just want to say something about how elegantly "LTO" kerns, at least in this font and browser. Gives me a warm fuzzy feeling every time I type it. |
OK, lets try to enable it for develop. I'd like us to run full CI on both develop and release (@jamesbeyond). I'm assuming we will go for |
A couple of updates:
|
The rebuild time above was generated with the same build command after invoking |
Mmm, that's a pretty hefty rebuild time increase. (But I could probably live with it if I get assigned a desktop workstation. All the cool kids around here seem to have one these days.) It may be that But it seems any form of parallel link could be adding new dependency complications :( |
Not much progress in testing LTO performance on Windows so far. Got a weird error during the link stage:
I'm trying to resolve that. |
@0xc0170 sorry for the late update! I guess the CI will have to be restarted after another review round. |
Test run: FAILEDSummary: 1 of 4 test jobs failed Failed test jobs:
|
CI restarted |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
@ARMmbed/mbed-os-tools Can you please review again? If this is approved today, can be in alpha2 |
ping @mark-edgeworth, what do you think the changes this time? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a great improvement over the cache-file approach. Looks fine to me now.
It looks like this issue: ARMmbed/mbed-cli#942 may be related to this fix. @jamesbeyond can you comment if so? |
I hope Filip's change about re-ordering the symbols could fix this issue 😃 |
Sorry but that workaround is applied only if |
We are release alpha2, this will be merged tomorrow morning once we are good to go with the release |
I've tried the lto extension, it produces 222 messages like:
In some other issue I found that this is a 'cosmetic' error, but having >200 messages is annoying. Will updated tools suppress this message? The complete line in the map file is:
Another problem I found is that lto and minimal-printf linker extensions do not work together. |
@JojoS62 Thanks for the feedback. Can you create an issue report with details? We can then triage this and review |
Yes, I will prepare it. |
When switching over to Mbed OS 6.0 Alpha 3 the most tight optimized version of Client Lite stopped compiling. A mysterious error popped up complaining "-c" option is not supported. After some git bisecting the offending commit was found a issue was back-traced to some workarounds done to avoid some LTO issues with GCC. ARMmbed/mbed-os#11856 Issue is that the gcc.py changes done now expand the parameters to linked from the ld-block and common-block. THe linker does not recognize the "-c" option expanded there from the common-block. However, we can work around this issue the same way the original Mbed OS PR did - split the -c out to asm, c and cxx blocks. This works with both Mbed OS 5.15.x and Mbed OS 6.0.0 Alpha 3. More details in Mbed OS issue [#12871](ARMmbed/mbed-os#12781)
Description (required)
Summary of change (What the change is for and why)
Added an
lto
build profile extension forGCC_ARM
toolchain.-flto
tocommon
flags,tools/toolchains/gcc.py
to usecommon
flags at link time,markedmain()
withMBED_USED
attribute,-u main
told
flags.Results
arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 9-2019-q4-major) 9.2.1 20191025 (release) [ARM/arm-9-branch revision 277599]
mbed-cloud-client-example@4.3.0, mbed-os-5.15.1, GCC9, Linux host
release
release
+lto
mbed-cloud-client-example@4.3.0, mbed-os-5.15.1, GCC9, Windows host
release
release
+lto
Build commands used to produce the above results
Documentation (Details of any document updates required)
Pull request type (required)
Test results (required)
Reviewers (optional)
@jamesbeyond @kjbracey-arm @maciejbocianski
Release Notes (required for feature/major PRs)
Summary of changes
Add a build profile extension with the link-time optimizer enabled for GCC_ARM toolchain.
Impact of changes
Migration actions required
The minimal required version of the
GCC_ARM
is now the GNU Arm Embedded Toolchain Version 9-2019-q4-major. EarlierGCC_ARM
versions can cause various issues when the-flto
flag is used, e.g. a platform specific error during the final link stage on Windows hosts with GCC8.The
noinline
attribute has to be used for every function that must be placed into a specific section (specified with asection(".section_name")
attribute). In general, when a function is considered for inlining, thesection
attribute is always ignored. However, with the link-time optimizer enabled, the chances for inlining are much higher because the inliner works across multiple translation units. As a result, the output sections' sizes change compared to a non-lto build. This may lead to asection ".section_name" will not fit in region "region_name"
type errors.The
common
flags defined for theGCC_ARM
toolchain are now appended to theld
flags during a mbed-cli build. Previously, thecommon
flags were appended only toasm
,c
andcxx
flags. Having the same flags for the compiler and the linker is required when using the link-time optimizer (which is the case for thelto
build profile extension). Any options unrecognized by the linker should be moved fromcommon
toasm
,c
orcxx
.