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

Make ARMC5 and IAR develop profile also size optimized #10813

Merged
merged 2 commits into from Jul 4, 2019

Conversation

@JanneKiiskila
Copy link
Contributor

commented Jun 12, 2019

Description

Due to some historical reasons ARMC 5 compiler behaves very
differently compared to others (GCC, IAR, ARM C 6) as it optimizes
performance rather than size. The compilers should behave the same
way with the same profile, thus ARM C 5 should also drive towards
size.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@kjbracey-arm @bulislaw @0xc0170

Release Notes

Due to some historical reasons ARMC 5 compiler behaves very
differently compared to others (GCC, IAR, ARM C 6) as it optimizes
performance rather than size (like the others).

All compilers should behave the same way with the same profile,
thus ARM C 5 should also drive towards size (space).
Copy link
Contributor

left a comment

As far as I know, the original motivation for this was that ARM C 5 Otime produced similar code size to GCC 4 Ospace, and there was no point taking even less ROM than GCC, if we were testing all 3 toolchains in parallel - if GCC Ospace fitted then ARM C Otime would also fit.

GCC 6 and later produce much better ARM code, so ARM C needs Ospace to match it.

Copy link
Contributor

left a comment

IAR could also use "-Ohz" instead.

I would also fix the Debug profile. With current Mbed OS profile, you cannot compile PDMC with Mesh stack using Mbed OS version of debug profile.

https://github.com/ARMmbed/mbed-cloud-client-example/blob/master/profiles/size.json
https://github.com/ARMmbed/mbed-cloud-client-example/blob/master/profiles/debug_size.json

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Previous profile tweak discussions for reference: #2600, #2973 and #5274.

Agree on IAR.

I think it makes sense for all toolchains to go for the same speed/size/balance setting, given the same profile name.

For comparison, "release" currently selects all toolchains as space, and "debug" is low/no optimisation for all.

Copy link
Member

left a comment

Looks good to me, I'm assuming it doesn't cause troubles with binaries in the tree. Probably should go to 5.14, but I'm happy to be challenged here.

IAR is also performance optimizing instead of size optimizing in
develop profile. Align also that (review feedback).
@JanneKiiskila JanneKiiskila changed the title Make ARMC5 develop profile also size optimized Make ARMC5 and IAR develop profile also size optimized Jun 12, 2019
@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

I am not asking this for 5.13, it's too late for that. 5.14 (or next feature release) is OK as a target.
IAR change also done based on review feedback (separate commit).

@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@bulislaw - should not the binary stuff (if there would be any incompatibility) be caught in CI, if there was any issue? Should not be though, this doesn't change the way the binaries are formatted etc. and for all we know - the binaries could already now be size optimized - that fully depends on what settings the developer creating the binaries used (we know for a fact that all bootloaders we provide are size optimized).

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Set for 5.14 , we got some time to get this tested on master. This is breaking change and should be set as one (PR type) + release notes addition needed. This should be also reviewed by tools team/web/studio team - as it changes the online compiler (they use develop profile as I recall).

cc @ARMmbed/mbed-os-tools @arekzaluski

Previous profile tweak discussions for reference: #2600, #2973 and #5274.

+1 for the references @kjbracey-arm .

@0xc0170 0xc0170 added needs: review and removed needs: CI labels Jun 13, 2019
@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

I would disagree on the "breaking change". It just changes what the compiler emphasizes. It should not break any code or cause malfunctions (with the exception of potential compiler bugs).

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Yes, it shouldn't be a breaking change, and it shouldn't affect binary compatibility, although I can imagine some sort of exporter/online tool glitch.

But it still isn't an actual bug fix, which is what rules it out of backport to patch release.

@mark-edgeworth

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

I'd agree that this shouldn't be a breaking change, but there are just too many combinations of program, exporter, toolchain, device and profile to make full testing a practical consideration. Consequently almost any change here has the potential for breaking something. If this does cause some sort of exporter/online compiler glitch then it is unlikely we'll see it until someone complains, at which point finding the problem will be much more difficult.

@adbridge adbridge added this to Needs Work in Test Jun 13, 2019
@adbridge adbridge moved this from Needs Work to Needs review in Test Jun 13, 2019
@adbridge adbridge added needs: CI and removed needs: review labels Jun 20, 2019
@adbridge

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

ci started

@mbed-ci

This comment has been minimized.

Copy link

commented Jun 21, 2019

Test run: FAILED

Summary: 6 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Will restart testing (internal CI error)

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Jun 25, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Test failure not related, known issue on master

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Restarted entire pipeline

@mbed-ci

This comment has been minimized.

Copy link

commented Jul 2, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 4
Build artifacts

@0xc0170 0xc0170 removed the needs: CI label Jul 3, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I believe this one is ready and will be merged later today

@0xc0170 0xc0170 merged commit 2dc562f into ARMmbed:master Jul 4, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8520 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Test
  
Needs review
8 participants
You can’t perform that action at this time.