On Tue, Aug 11, 2015 at 02:05:37PM +0100, Matthew Wahab wrote: > The Aarch64 backend implements the atomic operations on memory with the same > patterns iterated over logical and arithmetic operation. It also specifies > constraints on an operand for the operations but the constraints used are only > valid for the logical operations. This can lead to an ICE, with the compiler > unable to generate a valid instruction. > > This patch reworks the atomic operation patterns to select the > appropriate constraint for the operation. The logical operations take > the constraints specified by the current lconst_atomic mode iterator > while the arithmetic operations (plus, sub) take constraint "I". > > This patch also adds the test-case provided by Matthia Klose for the bugzilla > entry https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67143, slightly modified to > compile as plain C. > > Tested for arm-none-eabi with cross-compiled check-gcc and for > arm-none-linux-gnueabihf with native bootstrap and make check. > > Ok for trunk? > > Matthew > gcc/ > 2015-08-10 Matthew Wahab <matthew.wa...@arm.com> > > PR target/67143 > * config/aarch64/atomic.md (atomic_<optab><mode>): Replace > 'lconst_atomic' with 'const_atomic'. > (atomic_fetch_<optab><mode>): Likewise. > (atomic_<optab>_fetch<mode>): Likewise. > * config/aarch64/iterators.md (lconst-atomic): Move below > 'const_atomic'. > (const_atomic): New. > > gcc/testsuite/ > 2015-08-10 Matthew Wahab <matthew.wa...@arm.com> > Matthias Klose <d...@debian.org> > > PR target/67143 > * gcc.target/aarch64/pr67143.c: New
> diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md > index 5d7966d..d3d6af7 100644 > --- a/gcc/config/aarch64/iterators.md > +++ b/gcc/config/aarch64/iterators.md > @@ -345,9 +345,6 @@ > ;; Attribute to describe constants acceptable in logical operations > (define_mode_attr lconst [(SI "K") (DI "L")]) > > -;; Attribute to describe constants acceptable in atomic logical operations > -(define_mode_attr lconst_atomic [(QI "K") (HI "K") (SI "K") (DI "L")]) > - > ;; Map a mode to a specific constraint character. > (define_mode_attr cmode [(QI "q") (HI "h") (SI "s") (DI "d")]) > > @@ -843,6 +840,16 @@ > (plus "aarch64_plus_operand") > (minus "aarch64_plus_operand")]) > > +;; Constants acceptable for atomic operations. > +;; This definition must appear in this file before the iterators it refers > to. > +(define_code_attr const_atomic > + [(plus "I") (minus "I") I'm worried this still gives us a mismatch between constraints and predicates. The relevant predicates here are: (define_predicate "aarch64_plus_operand" (ior (match_operand 0 "register_operand") (match_operand 0 "aarch64_plus_immediate"))) (define_predicate "aarch64_plus_immediate" (and (match_code "const_int") (ior (match_test "aarch64_uimm12_shift (INTVAL (op))") (match_test "aarch64_uimm12_shift (-INTVAL (op))")))) But our constraint only permits: (define_constraint "I" "A constant that can be used with an ADD operation." (and (match_code "const_int") (match_test "aarch64_uimm12_shift (ival)"))) Does this mean we are now loading constants we don't need to in to registers? I don't think we could cause this to ICE - but are we generating less good code than we would like? Thanks, James