Skip to content

Make ARMC5 and IAR develop profile also size optimized #10813

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 4, 2019
Merged

Make ARMC5 and IAR develop profile also size optimized #10813

merged 2 commits into from
Jul 4, 2019

Conversation

JanneKiiskila
Copy link
Contributor

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

Make ARMC5 develop profile also size optimized
261e071 
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).
kjbracey
kjbracey approved these changes Jun 12, 2019
View reviewed changes
Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

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

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.

teetak01
teetak01 reviewed Jun 12, 2019
View reviewed changes
Copy link
Contributor

@teetak01 teetak01 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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.

bulislaw
bulislaw approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@bulislaw bulislaw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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.

Align also IAR - develop profile to be size optimized
7bfe0be 
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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor

0xc0170 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 .

@JanneKiiskila
Copy link
Contributor Author

JanneKiiskila commented Jun 13, 2019 •
edited
Loading

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

ci started

@mbed-ci
Copy link

mbed-ci 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
Copy link
Contributor

0xc0170 commented Jun 24, 2019

Will restart testing (internal CI error)

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 25, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci 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
Copy link
Contributor

0xc0170 commented Jun 26, 2019

Test failure not related, known issue on master

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 2, 2019

Restarted entire pipeline

@mbed-ci
Copy link

mbed-ci commented Jul 2, 2019

Test run: SUCCESS

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

@0xc0170
Copy link
Contributor

0xc0170 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
@teetak01 teetak01 mentioned this pull request Jul 30, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teetak01 teetak01 teetak01 left review comments

@bulislaw bulislaw bulislaw approved these changes

@kjbracey kjbracey kjbracey approved these changes

Assignees
No one assigned
Labels
release-version: 5.14.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@JanneKiiskila @kjbracey @0xc0170 @mark-edgeworth @adbridge @mbed-ci @bulislaw @teetak01