Re: Fix expand_expr_real_1 handling of BLKmode bitfield references

[Richard Biener <richard dot guenther at gmail dot com>]

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;
> +}