* [PATCH] Fix ICE when expanding incorrect shift counts (PR rtl-optimization/69764)
@ 2016-02-11 23:26 Jakub Jelinek
2016-02-12 1:08 ` Bernd Schmidt
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Jelinek @ 2016-02-11 23:26 UTC (permalink / raw)
To: gcc-patches
Hi!
When expanding shifts with invalid shift counts (negative or too large),
the shift count on the GIMPLE level is typically an int mode INTEGER_CST,
but when it is passed down through various layers up to
expand_binop_directly, we only have one known mode (other than operand
modes, but that is VOIDmode for CONST_INTs) - the mode of the first argument
(== result). But, we treat it as if even the shift count has that mode,
and either keep it as is (if the expander has that mode for the shift
count), or convert_modes it to the mode of the operand.
If the CONST_INT is too large, we can have a problem though, because it
could be e.g result of expand_normal of SImode value originally, but
we then treat it as valid HImode or QImode CONST_INT, and so either crash
in convert_modes, or later on when dealing with the shift count, as it
might not be valid for the mode we are expecting.
As expand_shift_1 and expand_binop seem to use GEN_INT for the shift count
in lots of places, rather than say gen_int_mode, I think this needs to be
fixed up only at the low level - in expand_binop_directly, which this patch
does. The common case, where the shift count is >= 0 and < bitsize,
are handled without need to call gen_int_mode.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2016-02-12 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/69764
* optabs.c (expand_binop_directly): For shift_optab_p, if xop1
is CONST_INT above scalar int mode's GET_MODE_BITSIZE (mode),
force it into mode.
* c-c++-common/pr69764.c: New test.
--- gcc/optabs.c.jj 2016-02-11 20:28:51.240492706 +0100
+++ gcc/optabs.c 2016-02-12 00:11:58.951795368 +0100
@@ -1006,6 +1006,14 @@ expand_binop_directly (machine_mode mode
xop0 = avoid_expensive_constant (xmode0, binoptab, 0, xop0, unsignedp);
if (!shift_optab_p (binoptab))
xop1 = avoid_expensive_constant (xmode1, binoptab, 1, xop1, unsignedp);
+ /* The mode of shift/rotate second operand is often different
+ from the mode of the operation, and for invalid shift counts xop1
+ might not be valid constant in mode, so the following convert_modes
+ might ICE on it. Fix it up here. */
+ else if (CONST_INT_P (xop1)
+ && SCALAR_INT_MODE_P (mode)
+ && UINTVAL (xop1) >= GET_MODE_BITSIZE (mode))
+ xop1 = gen_int_mode (INTVAL (xop1), mode);
/* In case the insn wants input operands in modes different from
those of the actual operands, convert the operands. It would
--- gcc/testsuite/c-c++-common/pr69764.c.jj 2016-02-12 00:00:54.950084697 +0100
+++ gcc/testsuite/c-c++-common/pr69764.c 2016-02-12 00:00:54.950084697 +0100
@@ -0,0 +1,38 @@
+/* PR rtl-optimization/69764 */
+/* { dg-do compile { target int32plus } } */
+
+unsigned char
+fn1 (unsigned char a)
+{
+ return a >> ~6; /* { dg-warning "right shift count is negative" } */
+}
+
+unsigned short
+fn2 (unsigned short a)
+{
+ return a >> ~6; /* { dg-warning "right shift count is negative" } */
+}
+
+unsigned int
+fn3 (unsigned int a)
+{
+ return a >> ~6; /* { dg-warning "right shift count is negative" } */
+}
+
+unsigned char
+fn4 (unsigned char a)
+{
+ return a >> 0xff03; /* { dg-warning "right shift count >= width of type" } */
+}
+
+unsigned short
+fn5 (unsigned short a)
+{
+ return a >> 0xff03; /* { dg-warning "right shift count >= width of type" } */
+}
+
+unsigned int
+fn6 (unsigned int a)
+{
+ return a >> 0xff03; /* { dg-warning "right shift count >= width of type" } */
+}
Jakub
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Fix ICE when expanding incorrect shift counts (PR rtl-optimization/69764)
2016-02-11 23:26 [PATCH] Fix ICE when expanding incorrect shift counts (PR rtl-optimization/69764) Jakub Jelinek
@ 2016-02-12 1:08 ` Bernd Schmidt
2016-02-12 9:41 ` [PATCH] Fix ICE when expanding incorrect shift counts (PR rtl-optimization/69764, take 2) Jakub Jelinek
0 siblings, 1 reply; 3+ messages in thread
From: Bernd Schmidt @ 2016-02-12 1:08 UTC (permalink / raw)
To: Jakub Jelinek, gcc-patches
On 02/12/2016 12:26 AM, Jakub Jelinek wrote:
> When expanding shifts with invalid shift counts (negative or too large),
> the shift count on the GIMPLE level is typically an int mode INTEGER_CST,
> but when it is passed down through various layers up to
> expand_binop_directly, we only have one known mode (other than operand
> modes, but that is VOIDmode for CONST_INTs) - the mode of the first argument
> (== result). But, we treat it as if even the shift count has that mode,
> and either keep it as is (if the expander has that mode for the shift
> count), or convert_modes it to the mode of the operand.
> If the CONST_INT is too large, we can have a problem though, because it
> could be e.g result of expand_normal of SImode value originally, but
> we then treat it as valid HImode or QImode CONST_INT, and so either crash
> in convert_modes, or later on when dealing with the shift count, as it
> might not be valid for the mode we are expecting.
> As expand_shift_1 and expand_binop seem to use GEN_INT for the shift count
> in lots of places, rather than say gen_int_mode, I think this needs to be
> fixed up only at the low level - in expand_binop_directly, which this patch
> does. The common case, where the shift count is >= 0 and < bitsize,
> are handled without need to call gen_int_mode.
Hmm, I'm finding this explanation somewhat confusing.
Isn't the problem just that shifts are unusual as binops, in that the
two operands can have different modes? We have the appropriate mode for
the shift count in xmode1, so I think we should just always rebuild a
CONST_INT in that mode. As in,
else if (CONST_INT_P (xop1))
/* Shifts can have different modes for the shift count, and the caller
might not have taken this into account when generating an
integer. */
xop1 = gen_int_mode (INTVAL (xop1), xmode1);
Ok with that change if it passes.
Bernd
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] Fix ICE when expanding incorrect shift counts (PR rtl-optimization/69764, take 2)
2016-02-12 1:08 ` Bernd Schmidt
@ 2016-02-12 9:41 ` Jakub Jelinek
0 siblings, 0 replies; 3+ messages in thread
From: Jakub Jelinek @ 2016-02-12 9:41 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: gcc-patches
On Fri, Feb 12, 2016 at 02:08:53AM +0100, Bernd Schmidt wrote:
> Isn't the problem just that shifts are unusual as binops, in that the two
> operands can have different modes? We have the appropriate mode for the
> shift count in xmode1, so I think we should just always rebuild a CONST_INT
> in that mode. As in,
>
> else if (CONST_INT_P (xop1))
> /* Shifts can have different modes for the shift count, and the caller
> might not have taken this into account when generating an integer.
> */
> xop1 = gen_int_mode (INTVAL (xop1), xmode1);
>
> Ok with that change if it passes.
That works, but I need to also ensure that convert_modes is not called on
it, as that would again assume the constant is in mode, while it is now in
xmode1. So, is this ok for trunk (if it passes bootstrap/regtest)?
2016-02-12 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/69764
* optabs.c (expand_binop_directly): For shift_optab_p, if xop1
is CONST_INT, recreate it in xmode1 and ensure convert_modes
will not be called on it.
* c-c++-common/pr69764.c: New test.
--- gcc/optabs.c.jj 2016-02-12 00:50:30.410240366 +0100
+++ gcc/optabs.c 2016-02-12 10:33:32.743492532 +0100
@@ -988,7 +988,7 @@ expand_binop_directly (machine_mode mode
from_mode, 1);
machine_mode xmode0 = insn_data[(int) icode].operand[1].mode;
machine_mode xmode1 = insn_data[(int) icode].operand[2].mode;
- machine_mode mode0, mode1, tmp_mode;
+ machine_mode mode0, mode1 = VOIDmode, tmp_mode;
struct expand_operand ops[3];
bool commutative_p;
rtx_insn *pat;
@@ -1006,6 +1006,13 @@ expand_binop_directly (machine_mode mode
xop0 = avoid_expensive_constant (xmode0, binoptab, 0, xop0, unsignedp);
if (!shift_optab_p (binoptab))
xop1 = avoid_expensive_constant (xmode1, binoptab, 1, xop1, unsignedp);
+ else if (CONST_INT_P (xop1))
+ {
+ /* Shifts can have different modes for the shift count, and the caller
+ might not have taken this into account when generating an integer. */
+ xop1 = gen_int_mode (INTVAL (xop1), xmode1);
+ mode1 = xmode1;
+ }
/* In case the insn wants input operands in modes different from
those of the actual operands, convert the operands. It would
@@ -1020,7 +1027,8 @@ expand_binop_directly (machine_mode mode
mode0 = xmode0;
}
- mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode;
+ if (mode1 == VOIDmode)
+ mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode;
if (xmode1 != VOIDmode && xmode1 != mode1)
{
xop1 = convert_modes (xmode1, mode1, xop1, unsignedp);
--- gcc/testsuite/c-c++-common/pr69764.c.jj 2016-02-12 10:27:34.522587995 +0100
+++ gcc/testsuite/c-c++-common/pr69764.c 2016-02-12 10:27:34.522587995 +0100
@@ -0,0 +1,38 @@
+/* PR rtl-optimization/69764 */
+/* { dg-do compile { target int32plus } } */
+
+unsigned char
+fn1 (unsigned char a)
+{
+ return a >> ~6; /* { dg-warning "right shift count is negative" } */
+}
+
+unsigned short
+fn2 (unsigned short a)
+{
+ return a >> ~6; /* { dg-warning "right shift count is negative" } */
+}
+
+unsigned int
+fn3 (unsigned int a)
+{
+ return a >> ~6; /* { dg-warning "right shift count is negative" } */
+}
+
+unsigned char
+fn4 (unsigned char a)
+{
+ return a >> 0xff03; /* { dg-warning "right shift count >= width of type" } */
+}
+
+unsigned short
+fn5 (unsigned short a)
+{
+ return a >> 0xff03; /* { dg-warning "right shift count >= width of type" } */
+}
+
+unsigned int
+fn6 (unsigned int a)
+{
+ return a >> 0xff03; /* { dg-warning "right shift count >= width of type" } */
+}
Jakub
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-02-12 9:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-11 23:26 [PATCH] Fix ICE when expanding incorrect shift counts (PR rtl-optimization/69764) Jakub Jelinek
2016-02-12 1:08 ` Bernd Schmidt
2016-02-12 9:41 ` [PATCH] Fix ICE when expanding incorrect shift counts (PR rtl-optimization/69764, take 2) Jakub Jelinek
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).