[PATCH v3 1/2] x86: Convert CONST_WIDE_INT/CONST_VECTOR to broadcast

H.J. Lu hjl.tools@gmail.com
Wed Jun 9 23:37:21 GMT 2021


On Wed, Jun 9, 2021 at 12:44 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> [For some reason this message didn't reach my gmail account]
>
> > 1. Update move expanders to convert the CONST_WIDE_INT and CONST_VECTO
> > operands to vector broadcast from an integer with AVX2.
> > 2. Add ix86_gen_scratch_sse_rtx to return a scratch SSE register which
> > won't increase stack alignment requirement and blocks transformation by
> > the combine pass.
> > 3. Update PR 87767 tests to expect integer broadcast instead of broadcast
> > from memory.
> > 4. Update avx512f_cond_move.c to expect integer broadcast.
>
> +  else if (TARGET_64BIT
> +   && ix86_broadcast (val, GET_MODE_BITSIZE (DImode),
> +      val_broadcast))
> +    {
> +      /* NB: MOVQ takes a 32-bit signed immediate operand.  */
> +      if (trunc_int_for_mode (val_broadcast, SImode) != val_broadcast)
> + return nullptr;
> +      broadcast_mode = DImode;
> +    }
> +  else
> +    return nullptr;
>
> We have MOVABS insn and movdi_internal knows when to switch between
> MOVQ and MOVABS.

Fixed in the v4 patch.

> +  if (!ix86_expand_vector_init_duplicate (false, vector_mode, target,
> +  GEN_INT (val_broadcast)))
> +    gcc_unreachable ();
>
> We are using:
>
> bool ok = ix86_expand_vector_init_duplicate (...);
> gcc_assert (ok);
>
> idiom throughout i386/. Let's keep it this way.

Fixed in the v4 patch.

> +  if (REGNO (target) < FIRST_PSEUDO_REGISTER)
> +    target = gen_rtx_REG (mode, REGNO (target));
> +  else
> +    target = convert_to_mode (mode, target, 1);
> +
>
> This is not needed. lowpart_subreg should do the trick when changing
> mode of hard regs (also see comment for ix86_gen_scratch_sse_rtx).

Fixed in the v4 patch.

> +  rtx first;
> +
> +  if (can_create_pseudo_p ()
> +      && GET_MODE_SIZE (mode) >= 16
> +      && GET_MODE_CLASS (mode) == MODE_VECTOR_INT
> +      && (MEM_P (op1)
> +  && SYMBOL_REF_P (XEXP (op1, 0))
> +  && CONSTANT_POOL_ADDRESS_P (XEXP (op1, 0)))
> +      && (first = ix86_broadcast_from_integer_constant (mode, op1)))
> +    {
> +      /* Broadcast to XMM/YMM/ZMM register from an integer constant.  */
> +      op1 = ix86_gen_scratch_sse_rtx (mode, false);
> +      if (!ix86_expand_vector_init_duplicate (false, mode, op1, first))
> + gcc_unreachable ();
> +      emit_move_insn (op0, op1);
> +      return;
>
> Please try to avoid assignment inside the condition. And also use
> "gcc_assert (ok)" here.

Fixed in the v4 patch.

> +/* Return a scratch register in MODE for vector load and store.  If
> +   CONSTANT_INT_BROADCAST is true, it is used to hold constant integer
> +   broadcast result.  */
> +
> +rtx
> +ix86_gen_scratch_sse_rtx (machine_mode mode,
> +  bool constant_int_broadcast)
>
> This function should always return hard reg, simply:
>
> return gen_rtx_REG (mode, (TARGET_64BIT
>   ? LAST_REX_SSE_REG : LAST_SSE_REG));
>
> The complications with pseudo does not bring us anything (at the end
> we need a hard reg anyway, and I guess reload knows quite well how to
> avoid used temporary).

Fixed in the v4 patch.

> The function can then be renamed to ix86_gen_scratch_sse_reg.

I'd like to keep the ix86_gen_scratch_sse_rtx name:

rtx
ix86_gen_scratch_sse_rtx (machine_mode mode)
{
  if (TARGET_SSE)
    return gen_rtx_REG (mode, (TARGET_64BIT
                               ? LAST_REX_SSE_REG
                               : LAST_SSE_REG));
  else
    return gen_reg_rtx (mode);
}

so that it can be used to implement the target hook for memset later.

> * gcc.target/i386/avx512f-broadcast-pr87767-1.c: Expect integer
> broadcast.
> * gcc.target/i386/avx512f-broadcast-pr87767-5.c: Likewise.
> * gcc.target/i386/avx512vl-broadcast-pr87767-1.c: Likewise.
> * gcc.target/i386/avx512vl-broadcast-pr87767-5.c: Likewise.
> * gcc.target/i386/avx512f_cond_move.c: Also pass
> -mprefer-vector-width=512 and expect integer broadcast.
>
> No review for the above changes for AVX512 tests, someone else should
> check if the new code is better here.

A small benchmark:

https://gitlab.com/x86-benchmarks/microbenchmark/-/tree/vpaddd/broadcast

shows that integer broadcast is faster than embedded memory broadcast:

$ make
gcc -g -I. -O2 -march=skylake-avx512   -c -o test.o test.c
gcc -g   -c -o memory.o memory.S
gcc -g   -c -o broadcast.o broadcast.S
gcc -o test test.o memory.o broadcast.o
./test
memory      : 425538
broadcast   : 375260
$

Thanks.

-- 
H.J.


More information about the Gcc-patches mailing list