This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] V12 patch #2 of 14, Refactor rs6000_adjust_vec_address & rs6000_split_vec_extract_var
Hi!
On Thu, Jan 09, 2020 at 07:04:10PM -0500, Michael Meissner wrote:
> 2020-01-09 Michael Meissner <meissner@linux.ibm.com>
>
> * config/rs6000/rs6000.c (get_vector_offset): New helper function
> to calculate the offset in memory from the start of a vector of a
> particular element. Add code to keep the element number in
> bounces if the element number is variable.
"within bounds"?
> +/* Return the offset within a memory object (MEM) of a vector type to a given
> + element within the vector (ELEMENT) with an element size (SCALAR_SIZE). If
> + the element is constant, we return a constant integer. Otherwise, we use a
> + base register temporary to calculate the offset after making it to fit
> + within the vector and scaling it. */
"masking it"?
> +static rtx
> +get_vector_offset (rtx mem, rtx element, rtx base_tmp, unsigned scalar_size)
> +{
> + if (CONST_INT_P (element))
> + return GEN_INT (INTVAL (element) * scalar_size);
> +
> + /* All insns should use the 'Q' constraint (address is a single register) if
> + the element number is not a constant. */
> + rtx addr = XEXP (mem, 0);
> + gcc_assert (REG_P (addr) || SUBREG_P (addr));
satisfies_constraint_Q (addr)
> + /* Mask the element to make sure the element number is between 0 and the
> + maximum number of elements - 1 so that we don't generate an address
> + outside the vector. */
But why is that the correct thing to do? Garbage in, garbage out is
perfectly fine? Or do we have (e.g.) builtins that specify this masking?
If so, please say that here.
> + emit_insn (gen_rtx_SET (base_tmp, and_op));
> +
> + /* Shift the element to get the byte offset from the element number. */
> + int shift = exact_log2 (scalar_size);
> + gcc_assert (shift >= 0);
> +
> + if (shift > 0)
> + {
> + rtx shift_op = gen_rtx_ASHIFT (Pmode, base_tmp, GEN_INT (shift));
> + emit_insn (gen_rtx_SET (base_tmp, shift_op));
> + }
This sets the same pseudo twice (or is it never a pseudo?), not a good
idea in general.
Okay for trunk, with comments improved please. Thanks!
Segher