Hi!
On Fri, Dec 20, 2019 at 06:24:57PM -0500, Michael Meissner wrote:
> This patch fixes the bug pointed out in the V10 patch review that the code
> modified an input argument to vector extract with a variable element number.
Great, thanks.
> With this patch applied, the compiler will signal an error. FWIW, I did build
> all of Spec 2017 and Spec 2006 with this patch applied, but not the others,
> and
> we did not get an assertion failure.
Please document (at the start of rs6000_adjust_vec_address, maybe in the
function comment even) which arguments can be the same register and which
not, that kind of thing? That makes it much simpler to check that all
callers are okay (and do that as well), but even more importantly it makes
it more likely that it won't come back to bite us later.
> --- gcc/config/rs6000/rs6000.c (revision 279549)
> +++ gcc/config/rs6000/rs6000.c (working copy)
> @@ -6757,6 +6757,8 @@ rs6000_adjust_vec_address (rtx scalar_re
>
> else
> {
> + /* If we are called from rs6000_split_vec_extract_var, base_tmp may
> + be the same as element. */
This comment isn't very useful here (it is confusing even, I'd say).
Move this comment up? Make it part of what I propose above?
> @@ -6825,6 +6827,11 @@ rs6000_adjust_vec_address (rtx scalar_re
>
> else
> {
> + /* Make sure base_tmp is not the same as element_offset. This
> + can happen if the element number is variable and the address
> + is not a simple address. Otherwise we lose the offset, and
> + double the address. */
> + gcc_assert (!reg_mentioned_p (base_tmp, element_offset));
Otherwise we ICE, certainly after adding the assert ;-)
Asserts often do not need documentation at all. If they do, usually
something unexpected is going on. Rewriting things a bit can help.
The comment isn't very exact, btw... "is not the same as"... ithe assert
actually tests if the base_tmp is used in element_offset at all; the latter
can be a more complicated construct.
> + /* Make sure base_tmp is not the same as element_offset. This can
> happen
> + if the element number is variable and the address is not a simple
> + address. Otherwise we lose the offset, and double the address. */
> + gcc_assert (!reg_mentioned_p (base_tmp, element_offset));
Same here.
> @@ -6902,9 +6913,10 @@ rs6000_split_vec_extract_var (rtx dest,
> int num_elements = GET_MODE_NUNITS (mode);
> rtx num_ele_m1 = GEN_INT (num_elements - 1);
>
> - emit_insn (gen_anddi3 (element, element, num_ele_m1));
> + /* Make sure the element number is in bounds. */
> gcc_assert (REG_P (tmp_gpr));
How does that make sure the number is in bounds?
In general, do asserts as early as practical?
Segher
