PR middle-end/104140: bootstrap ICE on riscv.
Commit Message
This patch resolves the P1 "ice-on-valid-code" regression boostrapping
GCC on risv-unknown-linux-gnu caused by my recent MULT_HIGHPART_EXPR
functionality. RISC-V differs from x86_64 and many targets by
supporting a usmusidi3 instruction, basically a widening multiply
where one operand is signed and the other is unsigned. Alas the
final version of my patch to recognize MULT_HIGHPART_EXPR didn't
sufficiently defend against the operands of WIDEN_MULT_EXPR having
different signedness. This is fixed by the two-line change to
tree-ssa-math-opts.c's convert_mult_to_highpart in the patch below.
The majority of the rest of the patch is to the documentation
(in tree.def and generic.texi). It turns out that WIDEN_MULT_EXPR
wasn't previously documented in generic.texi, let alone the slightly
unusual semantics of allowing mismatched (signed vs unsigned) operands.
This also clarifies that MULT_HIGHPART_EXPR currently requires the
signedness of operands to match [but this might change in future
release of GCC to support targets with usmul<mode>3_highpart.
The one final chunk of this patch (that is hopefully sufficiently
close to obvious for stage 4) is a similar (NULL pointer) sanity
check in riscv_cpu_cpp_builtins. Currently running cc1 from the
command line (or from gdb) without specifying -march results in a
segmentation fault (ICE). This is a minor annoyance tracking down
issues (in cross compilers) for riscv, and trivially fixed as below.
This patch has been tested both on x86_64-pc-linux-gnu with a full
make bootstrap and make -k check, and on a cross-compiler to
riscv-unknown-linux-gnu where I was able to confirm the new test
case now passes. Ok for mainline?
2022-01-22 Roger Sayle <roger@nextmovesoftware.com>
gcc/ChangeLog
* tree-ssa-math-opts.c (convert_mult_to_highpart): Check that the
operands of the widening multiplication are either both signed or
both unsigned, and abort the conversion if mismatched.
* doc/generic.texi (WIDEN_MULT_EXPR): Describe expression node.
(MULT_HIGHPART_EXPR): Clarify that operands must have the same
signedness.
* tree.def (MULT_HIGHPART_EXPR): Document both operands must have
integer types with the same precision and signedness.
(WIDEN_MULT_EXPR): Document that operands must have integer types
with the same precision, but possibly differing signedness.
* config/riscv/risc-v.c (riscv_cpu_cpp_builtins): Defend against
riscv_current_subset_list returning a NULL pointer (empty list).
gcc/testsuite/ChangeLog
* gcc.target/riscv/pr104140.c: New test case.
Thanks in advance (and sorry for the inconvenience).
Roger
--
Comments
> Am 21.01.2022 um 15:59 schrieb Roger Sayle <roger@nextmovesoftware.com>:
>
>
>
> This patch resolves the P1 "ice-on-valid-code" regression boostrapping
>
> GCC on risv-unknown-linux-gnu caused by my recent MULT_HIGHPART_EXPR
>
> functionality. RISC-V differs from x86_64 and many targets by
>
> supporting a usmusidi3 instruction, basically a widening multiply
>
> where one operand is signed and the other is unsigned. Alas the
>
> final version of my patch to recognize MULT_HIGHPART_EXPR didn't
>
> sufficiently defend against the operands of WIDEN_MULT_EXPR having
>
> different signedness. This is fixed by the two-line change to
>
> tree-ssa-math-opts.c's convert_mult_to_highpart in the patch below.
>
>
>
> The majority of the rest of the patch is to the documentation
>
> (in tree.def and generic.texi). It turns out that WIDEN_MULT_EXPR
>
> wasn't previously documented in generic.texi, let alone the slightly
>
> unusual semantics of allowing mismatched (signed vs unsigned) operands.
>
> This also clarifies that MULT_HIGHPART_EXPR currently requires the
>
> signedness of operands to match [but this might change in future
>
> release of GCC to support targets with usmul<mode>3_highpart.
>
>
>
> The one final chunk of this patch (that is hopefully sufficiently
>
> close to obvious for stage 4) is a similar (NULL pointer) sanity
>
> check in riscv_cpu_cpp_builtins. Currently running cc1 from the
>
> command line (or from gdb) without specifying -march results in a
>
> segmentation fault (ICE). This is a minor annoyance tracking down
>
> issues (in cross compilers) for riscv, and trivially fixed as below.
>
>
>
>
>
> This patch has been tested both on x86_64-pc-linux-gnu with a full
>
> make bootstrap and make -k check, and on a cross-compiler to
>
> riscv-unknown-linux-gnu where I was able to confirm the new test
>
> case now passes. Ok for mainline?
Ok.
Thanks,
Richard
>
>
>
>
> 2022-01-22 Roger Sayle <roger@nextmovesoftware.com>
>
>
>
> gcc/ChangeLog
>
> * tree-ssa-math-opts.c (convert_mult_to_highpart): Check that the
>
> operands of the widening multiplication are either both signed or
>
> both unsigned, and abort the conversion if mismatched.
>
> * doc/generic.texi (WIDEN_MULT_EXPR): Describe expression node.
>
> (MULT_HIGHPART_EXPR): Clarify that operands must have the same
>
> signedness.
>
> * tree.def (MULT_HIGHPART_EXPR): Document both operands must have
>
> integer types with the same precision and signedness.
>
> (WIDEN_MULT_EXPR): Document that operands must have integer types
>
> with the same precision, but possibly differing signedness.
>
> * config/riscv/risc-v.c (riscv_cpu_cpp_builtins): Defend against
>
> riscv_current_subset_list returning a NULL pointer (empty list).
>
>
>
> gcc/testsuite/ChangeLog
>
> * gcc.target/riscv/pr104140.c: New test case.
>
>
>
>
>
> Thanks in advance (and sorry for the inconvenience).
>
> Roger
>
> --
>
>
>
>
>
> <patch.txt>
@@ -108,6 +108,9 @@ riscv_cpu_cpp_builtins (cpp_reader *pfile)
builtin_define_with_int_value ("__riscv_arch_test", 1);
const riscv_subset_list *subset_list = riscv_current_subset_list ();
+ if (!subset_list)
+ return;
+
size_t max_ext_len = 0;
/* Figure out the max length of extension name for reserving buffer. */
@@ -1318,6 +1318,7 @@ The type of the node specifies the alignment of the access.
@tindex PLUS_EXPR
@tindex MINUS_EXPR
@tindex MULT_EXPR
+@tindex WIDEN_MULT_EXPR
@tindex MULT_HIGHPART_EXPR
@tindex RDIV_EXPR
@tindex TRUNC_DIV_EXPR
@@ -1532,10 +1533,18 @@ one operand is of floating type and the other is of integral type.
The behavior of these operations on signed arithmetic overflow is
controlled by the @code{flag_wrapv} and @code{flag_trapv} variables.
+@item WIDEN_MULT_EXPR
+This node represents a widening multiplication. The operands have
+integral types with same @var{b} bits of precision, producing an
+integral type result with at least @math{2@var{b}} bits of precision.
+The behaviour is equivalent to extending both operands, possibly of
+different signedness, to the result type, then multiplying them.
+
@item MULT_HIGHPART_EXPR
This node represents the ``high-part'' of a widening multiplication.
For an integral type with @var{b} bits of precision, the result is
the most significant @var{b} bits of the full @math{2@var{b}} product.
+Both operands must have the same precision and same signedness.
@item RDIV_EXPR
This node represents a floating point division operation.
new file mode 100644
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -march=rv32im -mabi=ilp32" } */
+int x;
+unsigned u, v;
+void f (void)
+{
+ long long y = x;
+ u = y * v >> 32;
+}
+void g (void) { f (); }
+
@@ -4608,6 +4608,10 @@ convert_mult_to_highpart (gassign *stmt, gimple_stmt_iterator *gsi)
if (bits < prec || bits >= 2 * prec)
return false;
+ /* For the time being, require operands to have the same sign. */
+ if (unsignedp != TYPE_UNSIGNED (TREE_TYPE (mop2)))
+ return false;
+
machine_mode mode = TYPE_MODE (optype);
optab tab = unsignedp ? umul_highpart_optab : smul_highpart_optab;
if (optab_handler (tab, mode) == CODE_FOR_nothing)
@@ -700,7 +700,9 @@ DEFTREECODE (POINTER_PLUS_EXPR, "pointer_plus_expr", tcc_binary, 2)
DEFTREECODE (POINTER_DIFF_EXPR, "pointer_diff_expr", tcc_binary, 2)
/* Highpart multiplication. For an integral type with precision B,
- returns bits [2B-1, B] of the full 2*B product. */
+ returns bits [2B-1, B] of the full 2*B product. Both operands
+ and the result should have integer types of the same precision
+ and signedness. */
DEFTREECODE (MULT_HIGHPART_EXPR, "mult_highpart_expr", tcc_binary, 2)
/* Division for integer result that rounds the quotient toward zero. */
@@ -1349,10 +1351,12 @@ DEFTREECODE (WIDEN_SUM_EXPR, "widen_sum_expr", tcc_binary, 2)
DEFTREECODE (SAD_EXPR, "sad_expr", tcc_expression, 3)
/* Widening multiplication.
- The two arguments are of type t1.
- The result is of type t2, such that t2 is at least twice
- the size of t1. WIDEN_MULT_EXPR is equivalent to first widening (promoting)
- the arguments from type t1 to type t2, and then multiplying them. */
+ The two arguments are of type t1 and t2, both integral types that
+ have the same precision, but possibly different signedness.
+ The result is of integral type t3, such that t3 is at least twice
+ the size of t1/t2. WIDEN_MULT_EXPR is equivalent to first widening
+ (promoting) the arguments from type t1 to type t3, and from t2 to
+ type t3 and then multiplying them. */
DEFTREECODE (WIDEN_MULT_EXPR, "widen_mult_expr", tcc_binary, 2)
/* Widening multiply-accumulate.