| Message ID | 87bmcxabec.fsf@linaro.org |
|---|---|
| State | Accepted |
| Commit | 8566678b9da3db996f7566ecb691be07ff376c8f |
| Headers | show |
| Series | Fix expand_expr_real_1 handling of BLKmode bitfield references | expand |
On Wed, May 30, 2018 at 8:46 AM Richard Sandiford <richard.sandiford@linaro.org> wrote:> > The handling of bitfield references in expand_expr_real_1 includes: > > machine_mode ext_mode = mode; > > if (ext_mode == BLKmode > && ! (target != 0 && MEM_P (op0) > && MEM_P (target) > && multiple_p (bitpos, BITS_PER_UNIT))) > ext_mode = int_mode_for_size (bitsize, 1).else_blk (); > > if (ext_mode == BLKmode) > { > [...] > gcc_assert (MEM_P (op0) > > Here "mode" is the TYPE_MODE of the result, so when mode == BLKmode, > the target must be a MEM if nonnull, since no other rtl objects can > have BLKmode. But there's no guarantee that the source value op0 is also > BLKmode and thus also a MEM: we can reach the assert for any source if > the bitsize being extracted is larger than the largest integer mode > (or larger than MAX_FIXED_MODE_SIZE). > > This triggered for SVE with -msve-vector-bits=512, where we could > sometimes try to extract a BLKmode value from a 512-bit vector, > and where int_mode_for_size would rightly fail for large bitsizes. > > The patch reuses the existing: > > /* Otherwise, if this is a constant or the object is not in memory > and need be, put it there. */ > else if (CONSTANT_P (op0) || (!MEM_P (op0) && must_force_mem)) > { > memloc = assign_temp (TREE_TYPE (tem), 1, 1); > emit_move_insn (memloc, op0); > op0 = memloc; > clear_mem_expr = true; > } > > to handle this case. > > Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf and > x86_64-linux-gnu. OK to install? Ok. Richard. > Richard > > > 2018-05-30 Richard Sandiford <richard.sandiford@linaro.org> > > gcc/ > * expr.c (expand_expr_real_1): Force the operand into memory if > its TYPE_MODE is BLKmode and if there is no integer mode for > the number of bits being extracted. > > gcc/testsuite/ > * gcc.target/aarch64/sve/extract_5.c: New test. > > Index: gcc/expr.c > =================================================================== > --- gcc/expr.c 2018-05-30 07:33:11.652009370 +0100 > +++ gcc/expr.c 2018-05-30 07:44:31.856060230 +0100 > @@ -10582,6 +10582,8 @@ expand_expr_real_1 (tree exp, rtx target > to a larger size. */ > must_force_mem = (offset > || mode1 == BLKmode > + || (mode == BLKmode > + && !int_mode_for_size (bitsize, 1).exists ()) > || maybe_gt (bitpos + bitsize, > GET_MODE_BITSIZE (mode2))); > > Index: gcc/testsuite/gcc.target/aarch64/sve/extract_5.c > =================================================================== > --- /dev/null 2018-04-20 16:19:46.369131350 +0100 > +++ gcc/testsuite/gcc.target/aarch64/sve/extract_5.c 2018-05-30 07:44:31.857060190 +0100 > @@ -0,0 +1,71 @@ > +/* Originally from gcc.dg/vect/vect-alias-check-10.c. */ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -ftree-vectorize -msve-vector-bits=512" } */ > + > +#define N 87 > +#define M 6 > + > +typedef signed char sc; > +typedef unsigned char uc; > +typedef signed short ss; > +typedef unsigned short us; > +typedef int si; > +typedef unsigned int ui; > +typedef signed long long sll; > +typedef unsigned long long ull; > + > +#define FOR_EACH_TYPE(M) \ > + M (sc) M (uc) \ > + M (ss) M (us) \ > + M (si) M (ui) \ > + M (sll) M (ull) \ > + M (float) M (double) > + > +#define TEST_VALUE(I) ((I) * 5 / 2) > + > +#define ADD_TEST(TYPE) \ > + void __attribute__((noinline, noclone)) \ > + test_##TYPE (TYPE *a, int step) \ > + { \ > + for (int i = 0; i < N; ++i) \ > + { \ > + a[i * step + 0] = a[i * step + 0] + 1; \ > + a[i * step + 1] = a[i * step + 1] + 2; \ > + a[i * step + 2] = a[i * step + 2] + 4; \ > + a[i * step + 3] = a[i * step + 3] + 8; \ > + } \ > + } \ > + void __attribute__((noinline, noclone)) \ > + ref_##TYPE (TYPE *a, int step) \ > + { \ > + for (int i = 0; i < N; ++i) \ > + { \ > + a[i * step + 0] = a[i * step + 0] + 1; \ > + a[i * step + 1] = a[i * step + 1] + 2; \ > + a[i * step + 2] = a[i * step + 2] + 4; \ > + a[i * step + 3] = a[i * step + 3] + 8; \ > + asm volatile (""); \ > + } \ > + } > + > +#define DO_TEST(TYPE) \ > + for (int j = -M; j <= M; ++j) \ > + { \ > + TYPE a[N * M], b[N * M]; \ > + for (int i = 0; i < N * M; ++i) \ > + a[i] = b[i] = TEST_VALUE (i); \ > + int offset = (j < 0 ? N * M - 4 : 0); \ > + test_##TYPE (a + offset, j); \ > + ref_##TYPE (b + offset, j); \ > + if (__builtin_memcmp (a, b, sizeof (a)) != 0) \ > + __builtin_abort (); \ > + } > + > +FOR_EACH_TYPE (ADD_TEST) > + > +int > +main (void) > +{ > + FOR_EACH_TYPE (DO_TEST) > + return 0; > +}
Index: gcc/expr.c =================================================================== --- gcc/expr.c 2018-05-30 07:33:11.652009370 +0100 +++ gcc/expr.c 2018-05-30 07:44:31.856060230 +0100 @@ -10582,6 +10582,8 @@ expand_expr_real_1 (tree exp, rtx target to a larger size. */ must_force_mem = (offset || mode1 == BLKmode + || (mode == BLKmode + && !int_mode_for_size (bitsize, 1).exists ()) || maybe_gt (bitpos + bitsize, GET_MODE_BITSIZE (mode2))); Index: gcc/testsuite/gcc.target/aarch64/sve/extract_5.c =================================================================== --- /dev/null 2018-04-20 16:19:46.369131350 +0100 +++ gcc/testsuite/gcc.target/aarch64/sve/extract_5.c 2018-05-30 07:44:31.857060190 +0100 @@ -0,0 +1,71 @@ +/* Originally from gcc.dg/vect/vect-alias-check-10.c. */ +/* { dg-do compile } */ +/* { dg-options "-O2 -ftree-vectorize -msve-vector-bits=512" } */ + +#define N 87 +#define M 6 + +typedef signed char sc; +typedef unsigned char uc; +typedef signed short ss; +typedef unsigned short us; +typedef int si; +typedef unsigned int ui; +typedef signed long long sll; +typedef unsigned long long ull; + +#define FOR_EACH_TYPE(M) \ + M (sc) M (uc) \ + M (ss) M (us) \ + M (si) M (ui) \ + M (sll) M (ull) \ + M (float) M (double) + +#define TEST_VALUE(I) ((I) * 5 / 2) + +#define ADD_TEST(TYPE) \ + void __attribute__((noinline, noclone)) \ + test_##TYPE (TYPE *a, int step) \ + { \ + for (int i = 0; i < N; ++i) \ + { \ + a[i * step + 0] = a[i * step + 0] + 1; \ + a[i * step + 1] = a[i * step + 1] + 2; \ + a[i * step + 2] = a[i * step + 2] + 4; \ + a[i * step + 3] = a[i * step + 3] + 8; \ + } \ + } \ + void __attribute__((noinline, noclone)) \ + ref_##TYPE (TYPE *a, int step) \ + { \ + for (int i = 0; i < N; ++i) \ + { \ + a[i * step + 0] = a[i * step + 0] + 1; \ + a[i * step + 1] = a[i * step + 1] + 2; \ + a[i * step + 2] = a[i * step + 2] + 4; \ + a[i * step + 3] = a[i * step + 3] + 8; \ + asm volatile (""); \ + } \ + } + +#define DO_TEST(TYPE) \ + for (int j = -M; j <= M; ++j) \ + { \ + TYPE a[N * M], b[N * M]; \ + for (int i = 0; i < N * M; ++i) \ + a[i] = b[i] = TEST_VALUE (i); \ + int offset = (j < 0 ? N * M - 4 : 0); \ + test_##TYPE (a + offset, j); \ + ref_##TYPE (b + offset, j); \ + if (__builtin_memcmp (a, b, sizeof (a)) != 0) \ + __builtin_abort (); \ + } + +FOR_EACH_TYPE (ADD_TEST) + +int +main (void) +{ + FOR_EACH_TYPE (DO_TEST) + return 0; +}
The handling of bitfield references in expand_expr_real_1 includes: machine_mode ext_mode = mode; if (ext_mode == BLKmode && ! (target != 0 && MEM_P (op0) && MEM_P (target) && multiple_p (bitpos, BITS_PER_UNIT))) ext_mode = int_mode_for_size (bitsize, 1).else_blk (); if (ext_mode == BLKmode) { [...] gcc_assert (MEM_P (op0) Here "mode" is the TYPE_MODE of the result, so when mode == BLKmode, the target must be a MEM if nonnull, since no other rtl objects can have BLKmode. But there's no guarantee that the source value op0 is also BLKmode and thus also a MEM: we can reach the assert for any source if the bitsize being extracted is larger than the largest integer mode (or larger than MAX_FIXED_MODE_SIZE). This triggered for SVE with -msve-vector-bits=512, where we could sometimes try to extract a BLKmode value from a 512-bit vector, and where int_mode_for_size would rightly fail for large bitsizes. The patch reuses the existing: /* Otherwise, if this is a constant or the object is not in memory and need be, put it there. */ else if (CONSTANT_P (op0) || (!MEM_P (op0) && must_force_mem)) { memloc = assign_temp (TREE_TYPE (tem), 1, 1); emit_move_insn (memloc, op0); op0 = memloc; clear_mem_expr = true; } to handle this case. Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf and x86_64-linux-gnu. OK to install? Richard 2018-05-30 Richard Sandiford <richard.sandiford@linaro.org> gcc/ * expr.c (expand_expr_real_1): Force the operand into memory if its TYPE_MODE is BLKmode and if there is no integer mode for the number of bits being extracted. gcc/testsuite/ * gcc.target/aarch64/sve/extract_5.c: New test.