| Message ID | 576CF030.1010600@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
Ping. https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01731.html Thanks, Kyrill On 24/06/16 09:32, Kyrill Tkachov wrote: > Hi all, > > In this PR we get an ICE when trying to emit a conditional move through noce_convert_multiple_sets. > The comment in the patch explains the situation but we get a two-instruction sequence like: > (insn 20 19 21 3 (set (reg:SI 89 [ _5 ]) > (reg:SI 88 [ _4 ])) wice.c:8 82 {*movsi_internal} > (nil)) > (insn 21 20 25 3 (set (reg:HI 90 [ a_lsm.10 ]) > (subreg:HI (reg:SI 89 [ _5 ]) 0)) wice.c:8 84 {*movhi_internal} > (nil)) > > The first instruction feeds the second, but the second takes the lowpart subreg of the first destination. > This leads to the noce_emit_cmove call taking as arguments the first SImode destination and the second HImode > destination. This causes an assertion failure deeper down the line. > > The solution in this patch is catch this case and wrap the first destination in a lowpart subreg so that the two > operands of the cmove have the same mode. > > Bootstrapped and tested on aarch64-none-linux-gnu, arm-none-linux-gnueabihf and x86_64-unknown-linux-gnu. > > Ok for trunk? > > Thanks, > Kyrill > > 2016-06-24 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR rtl-optimization/71594 > * ifcvt.c (noce_convert_multiple_sets): Wrap new_val or old_val > into subregs of appropriate mode before trying to emit a conditional > move. > > 2016-06-24 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR rtl-optimization/71594 > * gcc.dg/torture/pr71594.c: New test.
On 07/04/2016 12:28 PM, Kyrill Tkachov wrote: > Ping. > https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01731.html > > Thanks, > Kyrill > > On 24/06/16 09:32, Kyrill Tkachov wrote: >> Hi all, >> >> In this PR we get an ICE when trying to emit a conditional move >> through noce_convert_multiple_sets. >> The comment in the patch explains the situation but we get a >> two-instruction sequence like: >> (insn 20 19 21 3 (set (reg:SI 89 [ _5 ]) >> (reg:SI 88 [ _4 ])) wice.c:8 82 {*movsi_internal} >> (nil)) >> (insn 21 20 25 3 (set (reg:HI 90 [ a_lsm.10 ]) >> (subreg:HI (reg:SI 89 [ _5 ]) 0)) wice.c:8 84 {*movhi_internal} >> (nil)) > + > + /* We allow simple lowpart register subreg SET sources in > + bb_ok_for_noce_convert_multiple_sets. Be careful when processing > + sequences like: > + (set (reg:SI r1) (reg:SI r2)) > + (set (reg:HI r3) (subreg:HI (r1))) > + For the second insn new_val or old_val (r1 in this example) will be > + taken from the temporaries and have the wider mode which will not > + match with the mode of the other source of the conditional move, so > + we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI). > + Wrap the two cmove operands into subregs if appropriate to prevent > + that. */ > + if (GET_MODE (new_val) != GET_MODE (temp)) > + { > + machine_mode src_mode = GET_MODE (new_val); > + machine_mode dst_mode = GET_MODE (temp); > + gcc_assert (GET_MODE_SIZE (src_mode) > GET_MODE_SIZE (dst_mode)); > + new_val = lowpart_subreg (dst_mode, new_val, src_mode); The question I have would be what happens if you have the inverse of the sequence you expect, maybe with multi-word regs? (set (reg:SI 0) (reg:SI x)) (set (reg:DI y) (reg:DI 0)) That seems like it would fail the assert. Maybe this is something we need to catch in the bb_ok function. Bernd
Hi Bernd, On 04/07/16 12:08, Bernd Schmidt wrote: > > > On 07/04/2016 12:28 PM, Kyrill Tkachov wrote: >> Ping. >> https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01731.html >> >> Thanks, >> Kyrill >> >> On 24/06/16 09:32, Kyrill Tkachov wrote: >>> Hi all, >>> >>> In this PR we get an ICE when trying to emit a conditional move >>> through noce_convert_multiple_sets. >>> The comment in the patch explains the situation but we get a >>> two-instruction sequence like: >>> (insn 20 19 21 3 (set (reg:SI 89 [ _5 ]) >>> (reg:SI 88 [ _4 ])) wice.c:8 82 {*movsi_internal} >>> (nil)) >>> (insn 21 20 25 3 (set (reg:HI 90 [ a_lsm.10 ]) >>> (subreg:HI (reg:SI 89 [ _5 ]) 0)) wice.c:8 84 {*movhi_internal} >>> (nil)) > >> + >> + /* We allow simple lowpart register subreg SET sources in >> + bb_ok_for_noce_convert_multiple_sets. Be careful when processing >> + sequences like: >> + (set (reg:SI r1) (reg:SI r2)) >> + (set (reg:HI r3) (subreg:HI (r1))) >> + For the second insn new_val or old_val (r1 in this example) will be >> + taken from the temporaries and have the wider mode which will not >> + match with the mode of the other source of the conditional move, so >> + we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI). >> + Wrap the two cmove operands into subregs if appropriate to prevent >> + that. */ >> + if (GET_MODE (new_val) != GET_MODE (temp)) >> + { >> + machine_mode src_mode = GET_MODE (new_val); >> + machine_mode dst_mode = GET_MODE (temp); >> + gcc_assert (GET_MODE_SIZE (src_mode) > GET_MODE_SIZE (dst_mode)); >> + new_val = lowpart_subreg (dst_mode, new_val, src_mode); > > The question I have would be what happens if you have the inverse of the sequence you expect, maybe with multi-word regs? > > (set (reg:SI 0) (reg:SI x)) > (set (reg:DI y) (reg:DI 0)) > > That seems like it would fail the assert. Maybe this is something we need to catch in the bb_ok function. > That does seem like it could cause trouble but I couldn't think of how that sequence could appear or what its semantics would be. Would assigning to the SImode reg 0 in your example not touch the upper bits of the DImode value? In any case, bb_ok_for_noce_convert_multiple_sets doesn't keep track of dependencies between the instructions so I think the best place to handle this case would be in noce_convert_multiple_sets where instead of the assert in this patch we'd just end_sequence () and return FALSE. Would that be preferable? Thanks for the review, Kyrill > > Bernd >
On 07/04/2016 01:18 PM, Kyrill Tkachov wrote: > That does seem like it could cause trouble but I couldn't think of how > that sequence could appear or what its > semantics would be. Would assigning to the SImode reg 0 in your example > not touch the upper bits of the DImode value? No, multi-word subreg accesses are per-word. > In any case, bb_ok_for_noce_convert_multiple_sets doesn't keep track of > dependencies between the instructions > so I think the best place to handle this case would be in > noce_convert_multiple_sets where instead of the assert > in this patch we'd just end_sequence () and return FALSE. > Would that be preferable? That should at least work, and I'd be ok with that. Bernd
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index fd2951673fb6bd6d9e5d52cdb88765434a603fb6..3617de674561019a8dd155f13391c6eb3cbac1e4 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -3228,6 +3228,33 @@ noce_convert_multiple_sets (struct noce_if_info *if_info) if (if_info->then_else_reversed) std::swap (old_val, new_val); + + /* We allow simple lowpart register subreg SET sources in + bb_ok_for_noce_convert_multiple_sets. Be careful when processing + sequences like: + (set (reg:SI r1) (reg:SI r2)) + (set (reg:HI r3) (subreg:HI (r1))) + For the second insn new_val or old_val (r1 in this example) will be + taken from the temporaries and have the wider mode which will not + match with the mode of the other source of the conditional move, so + we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI). + Wrap the two cmove operands into subregs if appropriate to prevent + that. */ + if (GET_MODE (new_val) != GET_MODE (temp)) + { + machine_mode src_mode = GET_MODE (new_val); + machine_mode dst_mode = GET_MODE (temp); + gcc_assert (GET_MODE_SIZE (src_mode) > GET_MODE_SIZE (dst_mode)); + new_val = lowpart_subreg (dst_mode, new_val, src_mode); + } + if (GET_MODE (old_val) != GET_MODE (temp)) + { + machine_mode src_mode = GET_MODE (old_val); + machine_mode dst_mode = GET_MODE (temp); + gcc_assert (GET_MODE_SIZE (src_mode) > GET_MODE_SIZE (dst_mode)); + old_val = lowpart_subreg (dst_mode, old_val, src_mode); + } + /* Actually emit the conditional move. */ rtx temp_dest = noce_emit_cmove (if_info, temp, cond_code, x, y, new_val, old_val); diff --git a/gcc/testsuite/gcc.dg/torture/pr71594.c b/gcc/testsuite/gcc.dg/torture/pr71594.c new file mode 100644 index 0000000000000000000000000000000000000000..468a9f6891c92ff76520af0eee242f08b01ae0cf --- /dev/null +++ b/gcc/testsuite/gcc.dg/torture/pr71594.c @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-options "--param max-rtl-if-conversion-insns=2" } */ + +unsigned short a; +int b, c; +int *d; +void fn1() { + *d = 24; + for (; *d <= 65;) { + unsigned short *e = &a; + b = (a &= 0 <= 0) < (c ?: (*e %= *d)); + for (; *d <= 83;) + ; + } +}
Hi all, In this PR we get an ICE when trying to emit a conditional move through noce_convert_multiple_sets. The comment in the patch explains the situation but we get a two-instruction sequence like: (insn 20 19 21 3 (set (reg:SI 89 [ _5 ]) (reg:SI 88 [ _4 ])) wice.c:8 82 {*movsi_internal} (nil)) (insn 21 20 25 3 (set (reg:HI 90 [ a_lsm.10 ]) (subreg:HI (reg:SI 89 [ _5 ]) 0)) wice.c:8 84 {*movhi_internal} (nil)) The first instruction feeds the second, but the second takes the lowpart subreg of the first destination. This leads to the noce_emit_cmove call taking as arguments the first SImode destination and the second HImode destination. This causes an assertion failure deeper down the line. The solution in this patch is catch this case and wrap the first destination in a lowpart subreg so that the two operands of the cmove have the same mode. Bootstrapped and tested on aarch64-none-linux-gnu, arm-none-linux-gnueabihf and x86_64-unknown-linux-gnu. Ok for trunk? Thanks, Kyrill 2016-06-24 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR rtl-optimization/71594 * ifcvt.c (noce_convert_multiple_sets): Wrap new_val or old_val into subregs of appropriate mode before trying to emit a conditional move. 2016-06-24 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR rtl-optimization/71594 * gcc.dg/torture/pr71594.c: New test.