| Message ID | 20160211232646.GH3017@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
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
--- 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" } */ +}