* Be stricter about CONST_VECTOR operands
@ 2017-11-06 9:10 Richard Sandiford
2017-11-06 14:30 ` James Greenhalgh
2017-11-08 22:52 ` Jeff Law
0 siblings, 2 replies; 3+ messages in thread
From: Richard Sandiford @ 2017-11-06 9:10 UTC (permalink / raw)
To: gcc-patches
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.
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Be stricter about CONST_VECTOR operands
2017-11-06 9:10 Be stricter about CONST_VECTOR operands Richard Sandiford
@ 2017-11-06 14:30 ` James Greenhalgh
2017-11-08 22:52 ` Jeff Law
1 sibling, 0 replies; 3+ messages in thread
From: James Greenhalgh @ 2017-11-06 14:30 UTC (permalink / raw)
To: gcc-patches, richard.sandiford; +Cc: nd
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Be stricter about CONST_VECTOR operands
2017-11-06 9:10 Be stricter about CONST_VECTOR operands Richard Sandiford
2017-11-06 14:30 ` James Greenhalgh
@ 2017-11-08 22:52 ` Jeff Law
1 sibling, 0 replies; 3+ messages in thread
From: Jeff Law @ 2017-11-08 22:52 UTC (permalink / raw)
To: gcc-patches, richard.sandiford
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-11-08 22:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-06 9:10 Be stricter about CONST_VECTOR operands Richard Sandiford
2017-11-06 14:30 ` James Greenhalgh
2017-11-08 22:52 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).