| Message ID | alpine.LSU.2.11.1602121119020.31122@t29.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
On Fri, Feb 12, 2016 at 11:23:26AM +0100, Richard Biener wrote: > > I am testing the following patch which fixes PR69771 where the code > doesn't match the comment before it. We get to expand a QImode << QImode > shift but the shift amount was of type int and thus it was expanded > as SImode constant. Then > > /* In case the insn wants input operands in modes different from > those of the actual operands, convert the operands. It would > seem that we don't need to convert CONST_INTs, but we do, so > that they're properly zero-extended, sign-extended or truncated > for their mode. */ > > has to apply as we need to re-extend the VOIDmode CONST_INT for > QImode. But then mode1 is computed as 'mode' (QImode) which happens > to match what is expected even though the constant isn't valid. > > The fix is IMHO to always call convert_modes for VOIDmode ops > (if the target doesn't expect VOIDmode itself). > > Bootstrap and regtest running on x86_64-unknown-linux-gnu, ok for trunk? This looks like the PR69764 fix I've sent last night (and updated patch this morning). BTW, I wouldn't use a runtime test with clearly undefined behavior, especially not if it tests what the outcome of that UB is. > 2016-02-12 Richard Biener <rguenther@suse.de> > > PR rtl-optimization/69771 > * optabs.c (expand_binop_directly): Properly zero-/sign-extend > VOIDmode operands. > > * gcc.dg/torture/pr69771.c: New testcase. Jakub
On Fri, 12 Feb 2016, Jakub Jelinek wrote: > On Fri, Feb 12, 2016 at 11:23:26AM +0100, Richard Biener wrote: > > > > I am testing the following patch which fixes PR69771 where the code > > doesn't match the comment before it. We get to expand a QImode << QImode > > shift but the shift amount was of type int and thus it was expanded > > as SImode constant. Then > > > > /* In case the insn wants input operands in modes different from > > those of the actual operands, convert the operands. It would > > seem that we don't need to convert CONST_INTs, but we do, so > > that they're properly zero-extended, sign-extended or truncated > > for their mode. */ > > > > has to apply as we need to re-extend the VOIDmode CONST_INT for > > QImode. But then mode1 is computed as 'mode' (QImode) which happens > > to match what is expected even though the constant isn't valid. > > > > The fix is IMHO to always call convert_modes for VOIDmode ops > > (if the target doesn't expect VOIDmode itself). > > > > Bootstrap and regtest running on x86_64-unknown-linux-gnu, ok for trunk? > > This looks like the PR69764 fix I've sent last night (and updated patch > this morning). > BTW, I wouldn't use a runtime test with clearly undefined behavior, > especially not if it tests what the outcome of that UB is. Ah, indeed looks like a dup. Let's go with your version which had feedback from Bernd already. Might want to add my testcase (w/o the runtime outcome test). Richard. > > 2016-02-12 Richard Biener <rguenther@suse.de> > > > > PR rtl-optimization/69771 > > * optabs.c (expand_binop_directly): Properly zero-/sign-extend > > VOIDmode operands. > > > > * gcc.dg/torture/pr69771.c: New testcase. > > Jakub
On 02/12/2016 11:46 AM, Richard Biener wrote: > On Fri, 12 Feb 2016, Jakub Jelinek wrote: > >> On Fri, Feb 12, 2016 at 11:23:26AM +0100, Richard Biener wrote: >>> >>> I am testing the following patch which fixes PR69771 where the code >>> doesn't match the comment before it. We get to expand a QImode << QImode >>> shift but the shift amount was of type int and thus it was expanded >>> as SImode constant. Then >>> >>> /* In case the insn wants input operands in modes different from >>> those of the actual operands, convert the operands. It would >>> seem that we don't need to convert CONST_INTs, but we do, so >>> that they're properly zero-extended, sign-extended or truncated >>> for their mode. */ >>> >>> has to apply as we need to re-extend the VOIDmode CONST_INT for >>> QImode. But then mode1 is computed as 'mode' (QImode) which happens >>> to match what is expected even though the constant isn't valid. >>> >>> The fix is IMHO to always call convert_modes for VOIDmode ops >>> (if the target doesn't expect VOIDmode itself). >>> >>> Bootstrap and regtest running on x86_64-unknown-linux-gnu, ok for trunk? >> >> This looks like the PR69764 fix I've sent last night (and updated patch >> this morning). >> BTW, I wouldn't use a runtime test with clearly undefined behavior, >> especially not if it tests what the outcome of that UB is. > > Ah, indeed looks like a dup. Let's go with your version which had > feedback from Bernd already. Might want to add my testcase (w/o the > runtime outcome test). Actually, Richard I was just looking at Jakub's second patch and I think yours is better - on the first round of review I didn't notice that the convert_modes code is there and is documented to deal with the CONST_INT problem. If it completed testing I think you should commit it. Bernd
On Fri, Feb 12, 2016 at 01:15:11PM +0100, Bernd Schmidt wrote: > >Ah, indeed looks like a dup. Let's go with your version which had > >feedback from Bernd already. Might want to add my testcase (w/o the > >runtime outcome test). > > Actually, Richard I was just looking at Jakub's second patch and I think > yours is better - on the first round of review I didn't notice that the > convert_modes code is there and is documented to deal with the CONST_INT > problem. If it completed testing I think you should commit it. That patch doesn't look right to me. The code is there not just for shifts, but also for non-shifts, and for non-shifts, we know the arguments are in mode, we also know unsignedp, so if needed convert_modes can perform zero or sign extension. IMHO it is just shifts/rotates that are problematical, because what the second operand mode is and whether it is unsigned or signed is just less well defined, and then various backends have various requirements on it. Also, on some target for shift/rotate xmode1 might be already equal to mode, and in that case convert_modes would not be called, but still the CONST_INT might be originally from yet another mode and we'd still ICE. Jakub
On Fri, 12 Feb 2016, Jakub Jelinek wrote: > On Fri, Feb 12, 2016 at 01:15:11PM +0100, Bernd Schmidt wrote: > > >Ah, indeed looks like a dup. Let's go with your version which had > > >feedback from Bernd already. Might want to add my testcase (w/o the > > >runtime outcome test). > > > > Actually, Richard I was just looking at Jakub's second patch and I think > > yours is better - on the first round of review I didn't notice that the > > convert_modes code is there and is documented to deal with the CONST_INT > > problem. If it completed testing I think you should commit it. > > That patch doesn't look right to me. The code is there not just for shifts, > but also for non-shifts, and for non-shifts, we know the arguments are in > mode, we also know unsignedp, so if needed convert_modes can perform > zero or sign extension. IMHO it is just shifts/rotates that are > problematical, because what the second operand mode is and whether it is unsigned > or signed is just less well defined, and then various backends have various > requirements on it. Also, on some target for shift/rotate xmode1 might be > already equal to mode, and in that case convert_modes would not be called, > but still the CONST_INT might be originally from yet another mode and we'd > still ICE. But my patch should deal with all this. - mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode; - if (xmode1 != VOIDmode && xmode1 != mode1) + mode1 = GET_MODE (xop1); + if (xmode1 != mode1) { xop1 = convert_modes (xmode1, mode1, xop1, unsignedp); mode1 = xmode1; so if xop1 is not VOIDmode and already of xmode1 we won't do anything. But if it is VOIDmode (and xmode1 is not VOIDmode) we'll always do convert_modes. The only thing that can (hopefully not) happen is that xmode1 is VOIDmode but mode1 is not (maybe removing the xmode1 != VOIDmode check is a bit too optimistic here). Not sure if there can be valid patterns requesting a VOIDmode op ... (and not only accept CONST_INTs). Richard.
On Fri, 12 Feb 2016, Richard Biener wrote: > On Fri, 12 Feb 2016, Jakub Jelinek wrote: > > > On Fri, Feb 12, 2016 at 01:15:11PM +0100, Bernd Schmidt wrote: > > > >Ah, indeed looks like a dup. Let's go with your version which had > > > >feedback from Bernd already. Might want to add my testcase (w/o the > > > >runtime outcome test). > > > > > > Actually, Richard I was just looking at Jakub's second patch and I think > > > yours is better - on the first round of review I didn't notice that the > > > convert_modes code is there and is documented to deal with the CONST_INT > > > problem. If it completed testing I think you should commit it. > > > > That patch doesn't look right to me. The code is there not just for shifts, > > but also for non-shifts, and for non-shifts, we know the arguments are in > > mode, we also know unsignedp, so if needed convert_modes can perform > > zero or sign extension. IMHO it is just shifts/rotates that are > > problematical, because what the second operand mode is and whether it is unsigned > > or signed is just less well defined, and then various backends have various > > requirements on it. Also, on some target for shift/rotate xmode1 might be > > already equal to mode, and in that case convert_modes would not be called, > > but still the CONST_INT might be originally from yet another mode and we'd > > still ICE. > > But my patch should deal with all this. > > - mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode; > - if (xmode1 != VOIDmode && xmode1 != mode1) > + mode1 = GET_MODE (xop1); > + if (xmode1 != mode1) > { > xop1 = convert_modes (xmode1, mode1, xop1, unsignedp); > mode1 = xmode1; > > so if xop1 is not VOIDmode and already of xmode1 we won't do anything. > But if it is VOIDmode (and xmode1 is not VOIDmode) we'll always > do convert_modes. > > The only thing that can (hopefully not) happen is that xmode1 > is VOIDmode but mode1 is not (maybe removing the xmode1 != VOIDmode > check is a bit too optimistic here). Not sure if there can be > valid patterns requesting a VOIDmode op ... (and not only accept > CONST_INTs). Oh, bootstrap & testing went fine on x86_64-unknown-linux-gnu. If we can agree on the patch then I'll pick up the testcase from your patch (and adjust mine). Richard.
On Fri, Feb 12, 2016 at 01:48:36PM +0100, Richard Biener wrote: > But my patch should deal with all this. > > - mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode; > - if (xmode1 != VOIDmode && xmode1 != mode1) > + mode1 = GET_MODE (xop1); > + if (xmode1 != mode1) > { > xop1 = convert_modes (xmode1, mode1, xop1, unsignedp); > mode1 = xmode1; > > so if xop1 is not VOIDmode and already of xmode1 we won't do anything. > But if it is VOIDmode (and xmode1 is not VOIDmode) we'll always > do convert_modes. The case I'm worried about is if xop1 is a constant in narrower mode (let's say QImode), e.g. -15, unsignedp is 1, and xmode1 is wider. Then previously we'd zero extend it, thus get 0xf1, but with your patch we'll end up with -15 instead, because convert_modes will be called e.g. with (SImode, VOIDmode, xop1, 1) instead of (SImode, QImode, xop1, 1). Dunno if it is just hypothetical or real. Jakub
On Fri, 12 Feb 2016, Jakub Jelinek wrote: > On Fri, Feb 12, 2016 at 01:48:36PM +0100, Richard Biener wrote: > > But my patch should deal with all this. > > > > - mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode; > > - if (xmode1 != VOIDmode && xmode1 != mode1) > > + mode1 = GET_MODE (xop1); > > + if (xmode1 != mode1) > > { > > xop1 = convert_modes (xmode1, mode1, xop1, unsignedp); > > mode1 = xmode1; > > > > so if xop1 is not VOIDmode and already of xmode1 we won't do anything. > > But if it is VOIDmode (and xmode1 is not VOIDmode) we'll always > > do convert_modes. > > The case I'm worried about is if xop1 is a constant in narrower > mode (let's say QImode), e.g. -15, unsignedp is 1, and xmode1 is > wider. Then previously we'd zero extend it, thus get > 0xf1, but with your patch we'll end up with -15 instead, > because convert_modes will be called e.g. with (SImode, VOIDmode, xop1, 1) > instead of (SImode, QImode, xop1, 1). > Dunno if it is just hypothetical or real. But we don't know which mode xop1 was expanded with here. The only way to know would be passing down its original type (or its mode). That's the general issue with our modeless CONST_INTs. So yes, that case is sth to worry about (for all operations that can arrive in expand_binop_directly which can have different mode operands). Also note that unsignedp doesn't apply to op1 for shifts, only to op0. So eventually we shouldn't (ab-)use expand_binop for expanding shifts at all... Richard.
On Fri, Feb 12, 2016 at 02:45:40PM +0100, Richard Biener wrote: > > The case I'm worried about is if xop1 is a constant in narrower > > mode (let's say QImode), e.g. -15, unsignedp is 1, and xmode1 is > > wider. Then previously we'd zero extend it, thus get > > 0xf1, but with your patch we'll end up with -15 instead, > > because convert_modes will be called e.g. with (SImode, VOIDmode, xop1, 1) > > instead of (SImode, QImode, xop1, 1). > > Dunno if it is just hypothetical or real. > > But we don't know which mode xop1 was expanded with here. The only > way to know would be passing down its original type (or its mode). Well, most binary ops have the requirement that both modes are the same, which is why most of the expansion APIs for them pass around just a single mode, not two modes (or 3, if even the result could have different mode). And in that case we better have expanded the CONST_INTs with the right mode already. Just shifts are special. > So yes, that case is sth to worry about (for all operations that > can arrive in expand_binop_directly which can have different mode > operands). > > Also note that unsignedp doesn't apply to op1 for shifts, only to op0. > So eventually we shouldn't (ab-)use expand_binop for expanding shifts > at all... Perhaps; but please look at the latest patch I've posted, which IMHO does what your patch does for shift/rorate xop1 only, and keeps doing what it used to do otherwise. Jakub
Index: gcc/optabs.c =================================================================== --- gcc/optabs.c (revision 233369) +++ gcc/optabs.c (working copy) @@ -1013,15 +1013,15 @@ expand_binop_directly (machine_mode mode that they're properly zero-extended, sign-extended or truncated for their mode. */ - mode0 = GET_MODE (xop0) != VOIDmode ? GET_MODE (xop0) : mode; - if (xmode0 != VOIDmode && xmode0 != mode0) + mode0 = GET_MODE (xop0); + if (xmode0 != mode0) { xop0 = convert_modes (xmode0, mode0, xop0, unsignedp); mode0 = xmode0; } - mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode; - if (xmode1 != VOIDmode && xmode1 != mode1) + mode1 = GET_MODE (xop1); + if (xmode1 != mode1) { xop1 = convert_modes (xmode1, mode1, xop1, unsignedp); mode1 = xmode1; Index: gcc/testsuite/gcc.dg/torture/pr69771.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr69771.c (revision 0) +++ gcc/testsuite/gcc.dg/torture/pr69771.c (working copy) @@ -0,0 +1,15 @@ +/* { dg-do compile } */ + +unsigned char a = 5, c; +unsigned short b = 0; +unsigned d = 0x76543210; +void __attribute__((noinline)) +fn1() { c = d >> ~(a || ~b); } /* { dg-warning "shift count is negative" } */ + +int main() +{ + fn1(); + if (c != 1) + __builtin_abort (); + return 0; +}
I am testing the following patch which fixes PR69771 where the code doesn't match the comment before it. We get to expand a QImode << QImode shift but the shift amount was of type int and thus it was expanded as SImode constant. Then /* In case the insn wants input operands in modes different from those of the actual operands, convert the operands. It would seem that we don't need to convert CONST_INTs, but we do, so that they're properly zero-extended, sign-extended or truncated for their mode. */ has to apply as we need to re-extend the VOIDmode CONST_INT for QImode. But then mode1 is computed as 'mode' (QImode) which happens to match what is expected even though the constant isn't valid. The fix is IMHO to always call convert_modes for VOIDmode ops (if the target doesn't expect VOIDmode itself). Bootstrap and regtest running on x86_64-unknown-linux-gnu, ok for trunk? Thanks, Richard. 2016-02-12 Richard Biener <rguenther@suse.de> PR rtl-optimization/69771 * optabs.c (expand_binop_directly): Properly zero-/sign-extend VOIDmode operands. * gcc.dg/torture/pr69771.c: New testcase.