Re: [PATCH, GCC/ARM, stage4] Set mode for success result of atomic compare and swap

public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed

From: Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>
To: Thomas Preudhomme <thomas.preudhomme@foss.arm.com>,
	 Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>,
	Richard Earnshaw <richard.earnshaw@arm.com>,
	 "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH, GCC/ARM, stage4] Set mode for success result of atomic compare and swap
Date: Wed, 03 May 2017 09:45:00 -0000	[thread overview]
Message-ID: <5909A598.4020106@foss.arm.com> (raw)
In-Reply-To: <8a356417-3ccd-f6c7-ee69-7b19719b4c44@foss.arm.com>

Hi Thomas,

On 03/05/17 10:39, Thomas Preudhomme wrote:
> Hi Kyrill,
>
> On 19/04/17 14:34, Kyrill Tkachov wrote:
>> Hi Thomas,
>>
>> On 12/04/17 09:59, Thomas Preudhomme wrote:
>>> Hi,
>>>
>>> Currently atomic_compare_and_swap<mode>_1 define_insn do not have a mode
>>> set for the destination of the set indicating the success result of the
>>> instruction. This is because the operand can be either a CC_Z register
>>> (for 32-bit targets) or a SI register (for 16-bit Thumb targets). This
>>> result in lack of checking for the mode.
>>>
>>> This commit use a new CCSI iterator to solve this issue while avoiding
>>> duplication of the patterns. The insn name are kept unique by using
>>> attributes tied to the iterator (SIDI:mode and CCSI:arch) instead of
>>> usign the builtin mode attribute. Expander arm_expand_compare_and_swap
>>> is also adapted accordingly.
>>>
>>> ChangeLog entry is as follows:
>>>
>>> *** gcc/ChangeLog ***
>>>
>>> 2017-04-11  Thomas Preud'homme <thomas.preudhomme@arm.com>
>>>
>>>     * config/arm/iterators.md (CCSI): New mode iterator.
>>>     (arch): New mode attribute.
>>>     * config/arm/sync.md (atomic_compare_and_swap<mode>_1): Rename into ...
>>> (atomic_compare_and_swap<CCSI:arch><NARROW:mode>_1): This and ...
>>> (atomic_compare_and_swap<CCSI:arch><SIDI:mode>_1): This.  Use CCSI
>>>     code iterator for success result mode.
>>>     * config/arm/arm.c (arm_expand_compare_and_swap): Adapt code to use
>>>     the corresponding new insn generators.
>>>
>>> Testing: arm-none-eabi cross-compiler built successfully for ARMv8-M
>>> Mainline and Baseline without the lack of destination mode warning in
>>> sync.md. Testsuite show no regression.
>>>
>>
>> Thanks for fixing these warnings.
>> The code looks ok to me but
>> I'd like to make sure that the rest of the arm atomic targets are not adversely
>> affected,
>> so please also do a test run for ARMv7-A and ARMv8-A targets.
>> Also, a bootstrap is required as always.
>
> Hi Kyrill,
>
> Bootstrapped and ran the testsuite for both ARMv7-A and ARMv8-A in both ARM and Thumb mode without any regression. I've also verified that a number of atomic related testcases [1][2] get the same code generation for ARMv7-A in ARM and 
> Thumb mode as well as ARMv8-M Baseline.
>
> [1] For ARMv7-A ARM and Thumb mode, the following testcases were considered:
>
> gcc/testsuite/gcc.dg/atomic-compare-exchange-1.c
> gcc/testsuite/gcc.dg/atomic-compare-exchange-2.c
> gcc/testsuite/gcc.dg/atomic-compare-exchange-3.c
> gcc/testsuite/gcc.dg/atomic-exchange-1.c
> gcc/testsuite/gcc.dg/atomic-exchange-2.c
> gcc/testsuite/gcc.dg/atomic-exchange-3.c
> gcc/testsuite/gcc.dg/atomic-fence.c
> gcc/testsuite/gcc.dg/atomic-flag.c
> gcc/testsuite/gcc.dg/atomic-generic.c
> gcc/testsuite/gcc.dg/atomic-generic-aux.c
> gcc/testsuite/gcc.dg/atomic-invalid-2.c
> gcc/testsuite/gcc.dg/atomic-load-1.c
> gcc/testsuite/gcc.dg/atomic-load-2.c
> gcc/testsuite/gcc.dg/atomic-load-3.c
> gcc/testsuite/gcc.dg/atomic-lockfree.c
> gcc/testsuite/gcc.dg/atomic-lockfree-aux.c
> gcc/testsuite/gcc.dg/atomic-noinline.c
> gcc/testsuite/gcc.dg/atomic-noinline-aux.c
> gcc/testsuite/gcc.dg/atomic-op-1.c
> gcc/testsuite/gcc.dg/atomic-op-2.c
> gcc/testsuite/gcc.dg/atomic-op-3.c
> gcc/testsuite/gcc.dg/atomic-op-6.c
> gcc/testsuite/gcc.dg/atomic-store-1.c
> gcc/testsuite/gcc.dg/atomic-store-2.c
> gcc/testsuite/gcc.dg/atomic-store-3.c
> gcc/testsuite/g++.dg/ext/atomic-1.C
> gcc/testsuite/g++.dg/ext/atomic-2.C
> gcc/testsuite/gcc.target/arm/atomic-comp-swap-release-acquire-1.c
> gcc/testsuite/gcc.target/arm/atomic-op-acq_rel-1.c
> gcc/testsuite/gcc.target/arm/atomic-op-acquire-1.c
> gcc/testsuite/gcc.target/arm/atomic-op-char-1.c
> gcc/testsuite/gcc.target/arm/atomic-op-consume-1.c
> gcc/testsuite/gcc.target/arm/atomic-op-int-1.c
> gcc/testsuite/gcc.target/arm/atomic-op-relaxed-1.c
> gcc/testsuite/gcc.target/arm/atomic-op-release-1.c
> gcc/testsuite/gcc.target/arm/atomic-op-seq_cst-1.c
> gcc/testsuite/gcc.target/arm/atomic-op-short-1.c
> gcc/testsuite/gcc.target/arm/atomic_loaddi_1.c
> gcc/testsuite/gcc.target/arm/atomic_loaddi_4.c
> gcc/testsuite/gcc.target/arm/atomic_loaddi_7.c
> gcc/testsuite/gcc.target/arm/sync-1.c
> gcc/testsuite/gcc.target/arm/synchronize.c
> gcc/testsuite/gcc.target/arm/armv8-sync-comp-swap.c
> gcc/testsuite/gcc.target/arm/armv8-sync-op-acquire.c
> gcc/testsuite/gcc.target/arm/armv8-sync-op-full.c
> gcc/testsuite/gcc.target/arm/armv8-sync-op-release.c
> libstdc++-v3/testsuite/29_atomics/atomic/60658.cc
> libstdc++-v3/testsuite/29_atomics/atomic/62259.cc
> libstdc++-v3/testsuite/29_atomics/atomic/64658.cc
> libstdc++-v3/testsuite/29_atomics/atomic/65147.cc
> libstdc++-v3/testsuite/29_atomics/atomic/65913.cc
> libstdc++-v3/testsuite/29_atomics/atomic/70766.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/49445.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/constexpr.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/copy_list.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/default.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/direct_list.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/single_value.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/user_pod.cc
> libstdc++-v3/testsuite/29_atomics/atomic/operators/51811.cc
> libstdc++-v3/testsuite/29_atomics/atomic/operators/56011.cc
> libstdc++-v3/testsuite/29_atomics/atomic/operators/integral_assignment.cc
> libstdc++-v3/testsuite/29_atomics/atomic/operators/integral_conversion.cc
> libstdc++-v3/testsuite/29_atomics/atomic/operators/pointer_partial_void.cc
> libstdc++-v3/testsuite/29_atomics/atomic/requirements/base_classes.cc
> libstdc++-v3/testsuite/29_atomics/atomic/requirements/compare_exchange_lowering.cc
> libstdc++-v3/testsuite/29_atomics/atomic/requirements/explicit_instantiation/1.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/clear/1.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/1.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/56012.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/aggregate.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/default.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/requirements/standard_layout.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/requirements/trivial.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/test_and_set/explicit.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/test_and_set/implicit.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/60940.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/65147.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/cons/constexpr.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/cons/copy_list.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/cons/default.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/cons/direct_list.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/cons/single_value.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/operators/bitwise.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/operators/decrement.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/operators/increment.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/operators/integral_assignment.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/operators/integral_conversion.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/requirements/standard_layout.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/requirements/trivial.cc
> libstdc++-v3/testsuite/29_atomics/headers/atomic/functions_std_c++0x.cc
> libstdc++-v3/testsuite/29_atomics/headers/atomic/macros.cc
> libstdc++-v3/testsuite/29_atomics/headers/atomic/types_std_c++0x.cc
>
> [2] For ARMv8-M Baseline, the following testcases were considered:
>
> gcc/testsuite/gcc.dg/atomic-compare-exchange-1.c
> gcc/testsuite/gcc.dg/atomic-compare-exchange-2.c
> gcc/testsuite/gcc.dg/atomic-compare-exchange-3.c
> gcc/testsuite/gcc.dg/atomic-exchange-1.c
> gcc/testsuite/gcc.dg/atomic-exchange-2.c
> gcc/testsuite/gcc.dg/atomic-exchange-3.c
> gcc/testsuite/gcc.dg/atomic-fence.c
> gcc/testsuite/gcc.dg/atomic-flag.c
> gcc/testsuite/gcc.dg/atomic-generic.c
> gcc/testsuite/gcc.dg/atomic-generic-aux.c
> gcc/testsuite/gcc.dg/atomic-invalid-2.c
> gcc/testsuite/gcc.dg/atomic-load-1.c
> gcc/testsuite/gcc.dg/atomic-load-2.c
> gcc/testsuite/gcc.dg/atomic-load-3.c
> gcc/testsuite/gcc.dg/atomic-lockfree.c
> gcc/testsuite/gcc.dg/atomic-lockfree-aux.c
> gcc/testsuite/gcc.dg/atomic-noinline.c
> gcc/testsuite/gcc.dg/atomic-noinline-aux.c
> gcc/testsuite/gcc.dg/atomic-op-1.c
> gcc/testsuite/gcc.dg/atomic-op-2.c
> gcc/testsuite/gcc.dg/atomic-op-3.c
> gcc/testsuite/gcc.dg/atomic-op-6.c
> gcc/testsuite/gcc.dg/atomic-store-1.c
> gcc/testsuite/gcc.dg/atomic-store-2.c
> gcc/testsuite/gcc.dg/atomic-store-3.c
> gcc/testsuite/g++.dg/ext/atomic-1.C
> gcc/testsuite/g++.dg/ext/atomic-2.C
> gcc/testsuite/gcc.target/arm/atomic-comp-swap-release-acquire-3.c
> gcc/testsuite/gcc.target/arm/atomic-op-acq_rel-3.c
> gcc/testsuite/gcc.target/arm/atomic-op-acquire-3.c
> gcc/testsuite/gcc.target/arm/atomic-op-char-3.c
> gcc/testsuite/gcc.target/arm/atomic-op-consume-3.c
> gcc/testsuite/gcc.target/arm/atomic-op-int-3.c
> gcc/testsuite/gcc.target/arm/atomic-op-relaxed-3.c
> gcc/testsuite/gcc.target/arm/atomic-op-release-3.c
> gcc/testsuite/gcc.target/arm/atomic-op-seq_cst-3.c
> gcc/testsuite/gcc.target/arm/atomic-op-short-3.c
> gcc/testsuite/gcc.target/arm/sync-1.c
> gcc/testsuite/gcc.target/arm/synchronize.c
> gcc/testsuite/gcc.target/arm/armv8-sync-comp-swap.c
> gcc/testsuite/gcc.target/arm/armv8-sync-op-acquire.c
> gcc/testsuite/gcc.target/arm/armv8-sync-op-full.c
> gcc/testsuite/gcc.target/arm/armv8-sync-op-release.c
> libstdc++-v3/testsuite/29_atomics/atomic/60658.cc
> libstdc++-v3/testsuite/29_atomics/atomic/62259.cc
> libstdc++-v3/testsuite/29_atomics/atomic/64658.cc
> libstdc++-v3/testsuite/29_atomics/atomic/65147.cc
> libstdc++-v3/testsuite/29_atomics/atomic/65913.cc
> libstdc++-v3/testsuite/29_atomics/atomic/70766.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/49445.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/constexpr.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/copy_list.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/default.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/direct_list.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/single_value.cc
> libstdc++-v3/testsuite/29_atomics/atomic/cons/user_pod.cc
> libstdc++-v3/testsuite/29_atomics/atomic/operators/51811.cc
> libstdc++-v3/testsuite/29_atomics/atomic/operators/56011.cc
> libstdc++-v3/testsuite/29_atomics/atomic/operators/integral_assignment.cc
> libstdc++-v3/testsuite/29_atomics/atomic/operators/integral_conversion.cc
> libstdc++-v3/testsuite/29_atomics/atomic/operators/pointer_partial_void.cc
> libstdc++-v3/testsuite/29_atomics/atomic/requirements/base_classes.cc
> libstdc++-v3/testsuite/29_atomics/atomic/requirements/compare_exchange_lowering.cc
> libstdc++-v3/testsuite/29_atomics/atomic/requirements/explicit_instantiation/1.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/clear/1.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/1.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/56012.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/aggregate.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/default.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/requirements/standard_layout.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/requirements/trivial.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/test_and_set/explicit.cc
> libstdc++-v3/testsuite/29_atomics/atomic_flag/test_and_set/implicit.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/60940.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/65147.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/cons/constexpr.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/cons/copy_list.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/cons/default.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/cons/direct_list.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/cons/single_value.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/operators/bitwise.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/operators/decrement.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/operators/increment.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/operators/integral_assignment.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/operators/integral_conversion.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/requirements/standard_layout.cc
> libstdc++-v3/testsuite/29_atomics/atomic_integral/requirements/trivial.cc
> libstdc++-v3/testsuite/29_atomics/headers/atomic/functions_std_c++0x.cc
> libstdc++-v3/testsuite/29_atomics/headers/atomic/macros.cc
> libstdc++-v3/testsuite/29_atomics/headers/atomic/types_std_c++0x.cc
>
>>
>> Ok with that testing.
>
> Just to make sure, canyou confirm again that you're Ok for this to be commited in trunk with that amount of testing now that GCC 7.1 is released?
>

Thanks for the thorough testing, this is ok for trunk.

Kyrill

> Best regards,
>
> Thomas

next prev parent reply	other threads:[~2017-05-03  9:40 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-12  8:59 Thomas Preudhomme
2017-04-19  9:33 ` Thomas Preudhomme
2017-04-19 14:01 ` Kyrill Tkachov
2017-05-03  9:40   ` Thomas Preudhomme
2017-05-03  9:45     ` Kyrill Tkachov [this message]
2017-06-01 16:03       ` [arm-embedded] [PATCH, GCC, ARM/embedded-6/7-branch] " Thomas Preudhomme

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5909A598.4020106@foss.arm.com \
    --to=kyrylo.tkachov@foss.arm.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=ramana.radhakrishnan@arm.com \
    --cc=richard.earnshaw@arm.com \
    --cc=thomas.preudhomme@foss.arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Be sure your reply has a Subject: header at the top and a blank line before the message body.

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).