| Message ID | 87inenydq8.fsf@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Be stricter about CONST_VECTOR operands | expand |
On Mon, Nov 06, 2017 at 09:10:23AM +0000, Richard Sandiford wrote: > The recent gen_vec_duplicate patches used CONST_VECTOR for all > constants, but the documentation says: > > @findex const_vector > @item (const_vector:@var{m} [@var{x0} @var{x1} @dots{}]) > Represents a vector constant. The square brackets stand for the vector > containing the constant elements. @var{x0}, @var{x1} and so on are > the @code{const_int}, @code{const_double} or @code{const_fixed} elements. > > Both the AArch32 and AArch64 ports relied on the elements having > this form and would ICE if the element was something like a CONST > instead. This showed up as a failure in vect-102.c for both arm-eabi > and aarch64-elf (but not aarch64-linux-gnu, which is what the series > was tested on). > > The two obvious options were to redefine CONST_VECTOR to accept all > constants or make gen_vec_duplicate honour the existing documentation. > It looks like other code also assumes that integer CONST_VECTORs contain > CONST_INTs, so the patch does the latter. > > I deliberately didn't add an assert to gen_const_vec_duplicate > because it looks like the SPU port *does* expect to be able to create > CONST_VECTORs of symbolic constants. > > Also, I think the list above should include const_wide_int for vectors > of TImode and wider. > > The new routine takes a mode for consistency with the generators, > and because I think it does make sense to accept all constants for > variable-length: > > (const (vec_duplicate ...)) > > rather than have some rtxes for which we instead use: > > (vec_duplicate (const ...)) > > Tested on aarch64-elf, aarch64-linux-gnu, x86_64-linux-gnu and > powerpc64-linux-gnu. OK to install? For what it is worth, I can see why this would fix the bug in the AArch64 port, and the patch looks good to me (though I can't approve it). > (but not aarch64-linux-gnu, which is what the series was tested on). Presumably it would fail there for -mcmodel=tiny too, we just don't run that variant by default. Thanks, James > 2017-11-06 Richard Sandiford <richard.sandiford@linaro.org> > > gcc/ > * doc/rtl.texi (const_vector): Say that elements can be > const_wide_ints too. > * emit-rtl.h (valid_for_const_vec_duplicate_p): Declare. > * emit-rtl.c (valid_for_const_vec_duplicate_p): New function. > (gen_vec_duplicate): Use it instead of CONSTANT_P. > * optabs.c (expand_vector_broadcast): Likewise.
On 11/06/2017 02:10 AM, Richard Sandiford wrote: > The recent gen_vec_duplicate patches used CONST_VECTOR for all > constants, but the documentation says: > > @findex const_vector > @item (const_vector:@var{m} [@var{x0} @var{x1} @dots{}]) > Represents a vector constant. The square brackets stand for the vector > containing the constant elements. @var{x0}, @var{x1} and so on are > the @code{const_int}, @code{const_double} or @code{const_fixed} elements. > > Both the AArch32 and AArch64 ports relied on the elements having > this form and would ICE if the element was something like a CONST > instead. This showed up as a failure in vect-102.c for both arm-eabi > and aarch64-elf (but not aarch64-linux-gnu, which is what the series > was tested on). > > The two obvious options were to redefine CONST_VECTOR to accept all > constants or make gen_vec_duplicate honour the existing documentation. > It looks like other code also assumes that integer CONST_VECTORs contain > CONST_INTs, so the patch does the latter. > > I deliberately didn't add an assert to gen_const_vec_duplicate > because it looks like the SPU port *does* expect to be able to create > CONST_VECTORs of symbolic constants. > > Also, I think the list above should include const_wide_int for vectors > of TImode and wider. > > The new routine takes a mode for consistency with the generators, > and because I think it does make sense to accept all constants for > variable-length: > > (const (vec_duplicate ...)) > > rather than have some rtxes for which we instead use: > > (vec_duplicate (const ...)) > > Tested on aarch64-elf, aarch64-linux-gnu, x86_64-linux-gnu and > powerpc64-linux-gnu. OK to install? > > Richard > > > 2017-11-06 Richard Sandiford <richard.sandiford@linaro.org> > > gcc/ > * doc/rtl.texi (const_vector): Say that elements can be > const_wide_ints too. > * emit-rtl.h (valid_for_const_vec_duplicate_p): Declare. > * emit-rtl.c (valid_for_const_vec_duplicate_p): New function. > (gen_vec_duplicate): Use it instead of CONSTANT_P. > * optabs.c (expand_vector_broadcast): Likewise. OK. jeff
Index: gcc/doc/rtl.texi =================================================================== --- gcc/doc/rtl.texi 2017-11-06 08:56:27.608223621 +0000 +++ gcc/doc/rtl.texi 2017-11-06 09:01:44.150672938 +0000 @@ -1625,7 +1625,8 @@ accessed with @code{CONST_FIXED_VALUE_LO @item (const_vector:@var{m} [@var{x0} @var{x1} @dots{}]) Represents a vector constant. The square brackets stand for the vector containing the constant elements. @var{x0}, @var{x1} and so on are -the @code{const_int}, @code{const_double} or @code{const_fixed} elements. +the @code{const_int}, @code{const_wide_int}, @code{const_double} or +@code{const_fixed} elements. The number of units in a @code{const_vector} is obtained with the macro @code{CONST_VECTOR_NUNITS} as in @code{CONST_VECTOR_NUNITS (@var{v})}. Index: gcc/emit-rtl.h =================================================================== --- gcc/emit-rtl.h 2017-11-06 08:56:27.157323659 +0000 +++ gcc/emit-rtl.h 2017-11-06 09:01:44.151672938 +0000 @@ -438,6 +438,7 @@ get_max_uid (void) return crtl->emit.x_cur_insn_uid; } +extern bool valid_for_const_vec_duplicate_p (machine_mode, rtx); extern rtx gen_const_vec_duplicate (machine_mode, rtx); extern rtx gen_vec_duplicate (machine_mode, rtx); Index: gcc/emit-rtl.c =================================================================== --- gcc/emit-rtl.c 2017-11-06 08:56:27.608223621 +0000 +++ gcc/emit-rtl.c 2017-11-06 09:01:44.151672938 +0000 @@ -5757,6 +5757,17 @@ init_emit (void) #endif } +/* Return true if X is a valid element for a duplicated vector constant + of the given mode. */ + +bool +valid_for_const_vec_duplicate_p (machine_mode, rtx x) +{ + return (CONST_SCALAR_INT_P (x) + || CONST_DOUBLE_AS_FLOAT_P (x) + || CONST_FIXED_P (x)); +} + /* Like gen_const_vec_duplicate, but ignore const_tiny_rtx. */ static rtx @@ -5792,7 +5803,7 @@ gen_const_vec_duplicate (machine_mode mo rtx gen_vec_duplicate (machine_mode mode, rtx x) { - if (CONSTANT_P (x)) + if (valid_for_const_vec_duplicate_p (mode, x)) return gen_const_vec_duplicate (mode, x); return gen_rtx_VEC_DUPLICATE (mode, x); } Index: gcc/optabs.c =================================================================== --- gcc/optabs.c 2017-11-06 08:56:27.466923633 +0000 +++ gcc/optabs.c 2017-11-06 09:01:44.152672938 +0000 @@ -377,7 +377,7 @@ expand_vector_broadcast (machine_mode vm gcc_checking_assert (VECTOR_MODE_P (vmode)); - if (CONSTANT_P (op)) + if (valid_for_const_vec_duplicate_p (vmode, op)) return gen_const_vec_duplicate (vmode, op); /* ??? If the target doesn't have a vec_init, then we have no easy way
The recent gen_vec_duplicate patches used CONST_VECTOR for all constants, but the documentation says: @findex const_vector @item (const_vector:@var{m} [@var{x0} @var{x1} @dots{}]) Represents a vector constant. The square brackets stand for the vector containing the constant elements. @var{x0}, @var{x1} and so on are the @code{const_int}, @code{const_double} or @code{const_fixed} elements. Both the AArch32 and AArch64 ports relied on the elements having this form and would ICE if the element was something like a CONST instead. This showed up as a failure in vect-102.c for both arm-eabi and aarch64-elf (but not aarch64-linux-gnu, which is what the series was tested on). The two obvious options were to redefine CONST_VECTOR to accept all constants or make gen_vec_duplicate honour the existing documentation. It looks like other code also assumes that integer CONST_VECTORs contain CONST_INTs, so the patch does the latter. I deliberately didn't add an assert to gen_const_vec_duplicate because it looks like the SPU port *does* expect to be able to create CONST_VECTORs of symbolic constants. Also, I think the list above should include const_wide_int for vectors of TImode and wider. The new routine takes a mode for consistency with the generators, and because I think it does make sense to accept all constants for variable-length: (const (vec_duplicate ...)) rather than have some rtxes for which we instead use: (vec_duplicate (const ...)) Tested on aarch64-elf, aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu. OK to install? Richard 2017-11-06 Richard Sandiford <richard.sandiford@linaro.org> gcc/ * doc/rtl.texi (const_vector): Say that elements can be const_wide_ints too. * emit-rtl.h (valid_for_const_vec_duplicate_p): Declare. * emit-rtl.c (valid_for_const_vec_duplicate_p): New function. (gen_vec_duplicate): Use it instead of CONSTANT_P. * optabs.c (expand_vector_broadcast): Likewise.