| Message ID | 55C9F321.2080807@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
On 11/08/15 14:05, Matthew Wahab wrote: > Tested for arm-none-eabi with cross-compiled check-gcc and for > arm-none-linux-gnueabihf with native bootstrap and make check. This should have said: Tested for aarch64-none-eabi with cross-compiled check-gcc and for aarch64-none-linux-gnu with native bootstrap and make check. Matthew
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.wahab@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.wahab@arm.com> > Matthias Klose <doko@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
On Tue, 11 Aug 2015, Matthew Wahab wrote: > PR target/67143 > * gcc.target/aarch64/pr67143.c: New What's architecture-specific about this test? That is, why doesn't it just go in gcc.c-torture/compile (no dg- directives needed, automatically looped over optimization options)? Architecture-specific test directories should only be for tests that are actually architecture-specific (e.g. using architecture-specific options, or asm, or built-in functions, or scan-assembler for particular output), not for tests that can be built generically for all architectures and still test for the original bug.
diff --git a/gcc/config/aarch64/atomics.md b/gcc/config/aarch64/atomics.md index 1a38ac0..6e6be99 100644 --- a/gcc/config/aarch64/atomics.md +++ b/gcc/config/aarch64/atomics.md @@ -119,7 +119,7 @@ [(set (match_operand:ALLI 0 "aarch64_sync_memory_operand" "+Q") (unspec_volatile:ALLI [(atomic_op:ALLI (match_dup 0) - (match_operand:ALLI 1 "<atomic_op_operand>" "r<lconst_atomic>")) + (match_operand:ALLI 1 "<atomic_op_operand>" "r<const_atomic>")) (match_operand:SI 2 "const_int_operand")] ;; model UNSPECV_ATOMIC_OP)) (clobber (reg:CC CC_REGNUM)) @@ -164,7 +164,7 @@ (set (match_dup 1) (unspec_volatile:ALLI [(atomic_op:ALLI (match_dup 1) - (match_operand:ALLI 2 "<atomic_op_operand>" "r<lconst_atomic>")) + (match_operand:ALLI 2 "<atomic_op_operand>" "r<const_atomic>")) (match_operand:SI 3 "const_int_operand")] ;; model UNSPECV_ATOMIC_OP)) (clobber (reg:CC CC_REGNUM)) @@ -209,7 +209,7 @@ [(set (match_operand:ALLI 0 "register_operand" "=&r") (atomic_op:ALLI (match_operand:ALLI 1 "aarch64_sync_memory_operand" "+Q") - (match_operand:ALLI 2 "<atomic_op_operand>" "r<lconst_atomic>"))) + (match_operand:ALLI 2 "<atomic_op_operand>" "r<const_atomic>"))) (set (match_dup 1) (unspec_volatile:ALLI [(match_dup 1) (match_dup 2) 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") + (xor "<lconst_atomic>") (ior "<lconst_atomic>") + (and "<lconst_atomic>")]) + +;; Attribute to describe constants acceptable in atomic logical operations +(define_mode_attr lconst_atomic [(QI "K") (HI "K") (SI "K") (DI "L")]) + ;; ------------------------------------------------------------------- ;; Int Iterators. ;; ------------------------------------------------------------------- diff --git a/gcc/testsuite/gcc.target/aarch64/pr67143.c b/gcc/testsuite/gcc.target/aarch64/pr67143.c new file mode 100644 index 0000000..9335b33 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/pr67143.c @@ -0,0 +1,24 @@ +/* { dg-do compile } */ +/* { dg-options "-O3" } */ + +long a, c; +int b; +int d; +void ut_dbg_assertion_failed() __attribute__((noreturn)); +long dict_index_is_spatial(int *); +void btr_block_get_func(char *); +long btr_page_get_level_low(unsigned char *); +void btr_validate_level(long p1) { + unsigned char *e; + while (p1 != btr_page_get_level_low(e)) { + if (__builtin_expect(b, 0)) + ut_dbg_assertion_failed(); + if (dict_index_is_spatial(&d)) + while (c != 5535) { + __sync_add_and_fetch(&a, 536870912); + btr_block_get_func(""); + } + } + for (long i; i; ++i) + btr_validate_level(-i); +}