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

Assembler atomics #10147

Merged
merged 4 commits into from May 13, 2019

Conversation

Projects
None yet
9 participants
@kjbracey-arm
Copy link
Contributor

commented Mar 18, 2019

Description

Reimplement atomic code in inline assembly. This can improve optimisation, and avoids potential architectural problems with using LDREX/STREX intrinsics.

API further extended:

  • Bitwise operations (fetch_and/fetch_or/fetch_xor)
  • fetch_add and fetch_sub (like incr/decr, but returning old value - aligning with C++11)
  • compare_exchange_weak
  • Explicit memory order specification
  • Basic freestanding template overloads for C++

This gives our existing C implementation essentially all the functionality needed by C++11.

An actual Atomic<T> template based upon these C functions could follow.

Preliminary work for #10104.

Pull request type

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

Release Notes

  • Atomic APIs separated into mbed_atomic.h from mbed_critical.h. Code not including mbed.h may need to to check their includes.
  • Atomic fetch_add, fetch_sub, fetch_and, fetch_or, fetch_xor and compare_exchange_weak operations added.
  • Explicit memory ordering specification added (don't use unless familiar with C++11 memory model)
  • Freestanding atomic function templates added.

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Mar 18, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

@kjbracey-arm, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

I'm now looking at a full rework here, which will reduce the source size and turn it into assembler, with the possibility of better code generation.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Will close temporarily.

@kjbracey-arm kjbracey-arm reopened this Mar 25, 2019

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:atomic_bitwise branch from b8bf0f7 to cc1747a Mar 25, 2019

@kjbracey-arm kjbracey-arm changed the title Atomic bitwise operations Assembler atomics Mar 25, 2019

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:atomic_bitwise branch from cc1747a to a2a31ad Mar 25, 2019

@cmonr cmonr added the needs: review label Mar 25, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Hmm, this got nicked with the Astyle update bork.

Incoming fix from: #10221

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

Hmm... The astyle fix is missing this fix: #10222

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

@kjbracey-arm please re-base when you have time, it should resolve the astyle issue

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:atomic_bitwise branch 3 times, most recently from 827cc51 to 16eee15 Mar 26, 2019

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:atomic_bitwise branch 6 times, most recently from 53a3682 to 3c36dfe Mar 26, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@kjbracey-arm Please look at test failures

kjbracey-arm added some commits Jan 22, 2019

Assembler atomics
Reimplement atomic code in inline assembly. This can improve
optimisation, and avoids potential architectural problems with using
LDREX/STREX intrinsics.

API further extended:
* Bitwise operations (fetch_and/fetch_or/fetch_xor)
* fetch_add and fetch_sub (like incr/decr, but returning old value -
  aligning with C++11)
* compare_exchange_weak
* Explicit memory order specification
* Basic freestanding template overloads for C++

This gives our existing C implementation essentially all the functionality
needed by C++11.

An actual Atomic<T> template based upon these C functions could follow.
Add some atomic tests
These are platform tests, but rely on the RTOS to run multiple threads
to exercise it.

(The atomics are still useful in non-RTOS, to protect against interrupt
handlers, but testing versus other threads is easier. The implementation
is the same either way, so doesn't seem worth testing non-RTOS
specifically).
Critical tests: use atomics
Get rid of a volatile, and use atomics to synchronise with the interrupt
routine instead.

Useful as a non-RTOS basic compilation check for the atomics - the
fuller atomic test relies on the RTOS.
Add non-volatile atomic loads and stores
Volatile makes no real difference when we're using assembler, or locked
functions, but leaving it off could be more efficient for the basic
loads and stores. So add non-volatile overloads in C++ for them.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:atomic_bitwise branch from 1632263 to 2bb4045 Apr 26, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Thought I'd done enough fork tests to find all the glitches, but apparently not.

Fixed the flash_api.c build failure there for LPC55S69_NS. Didn't see anything else.

@adbridge adbridge added needs: review and removed needs: CI labels Apr 29, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@ARMmbed/mbed-os-core Could somebody please review this ?

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 2, 2019

@ARMmbed/mbed-os-core Could somebody please review this ?

@donatieng @bulislaw Can you help reviewing these?

@0xc0170 0xc0170 requested review from donatieng and bulislaw May 2, 2019

@bulislaw
Copy link
Member

left a comment

Looks good, but to be fair I didn't dive into the implementation file.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels May 6, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented May 7, 2019

Test run: FAILED

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

Failed test jobs:

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

This comment has been minimized.

Copy link
Member

commented May 10, 2019

CI restarted (py3 problem should be gone now)

@mbed-ci

This comment has been minimized.

Copy link

commented May 10, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@0xc0170

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Exporters restarted

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels May 13, 2019

@0xc0170 0xc0170 merged commit 3ea1c56 into ARMmbed:master May 13, 2019

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 Success
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 8639 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

@kjbracey-arm kjbracey-arm deleted the kjbracey-arm:atomic_bitwise branch May 14, 2019

@SanaMasood

This comment has been minimized.

Copy link

commented May 16, 2019

Build errors in mbed-os tests with ARMC5 toolchain started to come from 13th of May.
Seems like this pull request(#10147) is breaking everything. Build error started to appear in this file 'mbed_atomic_impl.h' with ARMC5 toolchain.

Can you please check why this pull request is effecting the build? and they seem related too!

[DEBUG] Output: "./mbed-os/platform/internal/mbed_atomic_impl.h", line 306: Error: #52: expected a macro parameter name
[DEBUG] Output: "./mbed-os/platform/internal/mbed_atomic_impl.h", line 498: Warning: #3732-D: use of this instruction in inline assembler is deprecated
[DEBUG] Output: "./mbed-os/platform/internal/mbed_atomic_impl.h", line 498: Error: #3081: expected end of line or a ";"

The build was passing on May 13, 2019 2:06:40 AM but after that it failed on May 13, 2019 2:43 PM (UK time)

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@SanaMasood - thanks for notification, I've made a PR for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.