| Message ID | f913b618-f708-2018-898b-57a8b84bb07f@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed,RISC-V] Don't reject constants in cmov condition | expand |
On 8/7/23 13:36, Jeff Law wrote:
> Fixes several missed conditional moves with the trunk.
I'm curious, how do you know what's missing. Does ifc have some stats
like autovec which explicitly reports missed opportunities ?
-Vineet
On 8/7/23 17:42, Vineet Gupta wrote: > > On 8/7/23 13:36, Jeff Law wrote: >> Fixes several missed conditional moves with the trunk. > > I'm curious, how do you know what's missing. Does ifc have some stats > like autovec which explicitly reports missed opportunities ? Xiao's testcases :-) I haven't even got to the point of trying our internal ones. yet. jeff
Hi Jeff, On 8/7/23 13:36, Jeff Law wrote: > > This test is too aggressive. Constants have VOIDmode, so we need to > let the through this phase of conditional move support. > > Fixes several missed conditional moves with the trunk. > > Committed to the trunk, As discussed this morning, this triggers an ICE when building glibc for rv64_zicond programs/ld-ctype.c:3977:1: error: unrecognizable insn: 3977 | } | ^ (insn 238 237 239 18 (set (reg:SI 727) (if_then_else:SI (eq:SI (reg:SI 725) (const_int 0 [0])) (const_int 0 [0]) (reg:SI 721))) -1 (nil)) during RTL pass: vregs programs/ld-ctype.c:3977:1: internal compiler error: in extract_insn, at recog.cc:2791 0x885de7 _fatal_insn(char const*, rtx_def const*, char const*, int, char const*) ../.././gcc/gcc/rtl-error.cc:108 0x885e09 _fatal_insn_not_found(rtx_def const*, char const*, int, char const*) ../.././gcc/gcc/rtl-error.cc:116 0x8846f9 extract_insn(rtx_insn*) ../.././gcc/gcc/recog.cc:2791 0xcf77ae instantiate_virtual_regs_in_insn ../.././gcc/gcc/function.cc:1610 0xcf77ae instantiate_virtual_regs ../.././gcc/gcc/function.cc:1983 0xcf77ae execute ../.././gcc/gcc/function.cc:2030 Here's the reduced reproducer. a, c; long b; d() { for (;;) if (a & (b < 8 ?: 1 << b)) c = 1; } I'll try to muster a fix :-) Thx, -Vineet
On 8/8/23 11:21, Vineet Gupta wrote: > Hi Jeff, > > On 8/7/23 13:36, Jeff Law wrote: >> >> This test is too aggressive. Constants have VOIDmode, so we need to >> let the through this phase of conditional move support. >> >> Fixes several missed conditional moves with the trunk. >> >> Committed to the trunk, > > As discussed this morning, this triggers an ICE when building glibc for > rv64_zicond > > programs/ld-ctype.c:3977:1: error: unrecognizable insn: > 3977 | } > | ^ > (insn 238 237 239 18 (set (reg:SI 727) > (if_then_else:SI (eq:SI (reg:SI 725) > (const_int 0 [0])) > (const_int 0 [0]) > (reg:SI 721))) -1 > (nil)) > during RTL pass: vregs > programs/ld-ctype.c:3977:1: internal compiler error: in extract_insn, at > recog.cc:2791 > 0x885de7 _fatal_insn(char const*, rtx_def const*, char const*, int, char > const*) ../.././gcc/gcc/rtl-error.cc:108 > 0x885e09 _fatal_insn_not_found(rtx_def const*, char const*, int, char > const*) ../.././gcc/gcc/rtl-error.cc:116 > 0x8846f9 extract_insn(rtx_insn*) ../.././gcc/gcc/recog.cc:2791 > 0xcf77ae instantiate_virtual_regs_in_insn ../.././gcc/gcc/function.cc:1610 > 0xcf77ae instantiate_virtual_regs ../.././gcc/gcc/function.cc:1983 > 0xcf77ae execute ../.././gcc/gcc/function.cc:2030 > > Here's the reduced reproducer. > > a, c; > long b; > d() { > for (;;) > if (a & (b < 8 ?: 1 << b)) > c = 1; > } > > I'll try to muster a fix :-) :-) That's even simpler than the one I just reduced. But I can see the same basic structure in my reduced case. I'll take it from here. I mucked something up, so I feel I ought to fix it. jeff
On 8/8/23 11:21, Vineet Gupta wrote: > Hi Jeff, > > On 8/7/23 13:36, Jeff Law wrote: >> >> This test is too aggressive. Constants have VOIDmode, so we need to >> let the through this phase of conditional move support. >> >> Fixes several missed conditional moves with the trunk. >> >> Committed to the trunk, > > As discussed this morning, this triggers an ICE when building glibc for > rv64_zicond > > programs/ld-ctype.c:3977:1: error: unrecognizable insn: > 3977 | } > | ^ > (insn 238 237 239 18 (set (reg:SI 727) > (if_then_else:SI (eq:SI (reg:SI 725) > (const_int 0 [0])) > (const_int 0 [0]) > (reg:SI 721))) -1 > (nil)) > during RTL pass: vregs > programs/ld-ctype.c:3977:1: internal compiler error: in extract_insn, at > recog.cc:2791 > 0x885de7 _fatal_insn(char const*, rtx_def const*, char const*, int, char > const*) ../.././gcc/gcc/rtl-error.cc:108 > 0x885e09 _fatal_insn_not_found(rtx_def const*, char const*, int, char > const*) ../.././gcc/gcc/rtl-error.cc:116 > 0x8846f9 extract_insn(rtx_insn*) ../.././gcc/gcc/recog.cc:2791 > 0xcf77ae instantiate_virtual_regs_in_insn ../.././gcc/gcc/function.cc:1610 > 0xcf77ae instantiate_virtual_regs ../.././gcc/gcc/function.cc:1983 > 0xcf77ae execute ../.././gcc/gcc/function.cc:2030 > > Here's the reduced reproducer. > > a, c; > long b; > d() { > for (;;) > if (a & (b < 8 ?: 1 << b)) > c = 1; > } > > I'll try to muster a fix :-) Should be fixed now, with a regression test in the suite. Thanks, jeff
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc index 279304afc19..5248dd3febe 100644 --- a/gcc/config/riscv/riscv.cc +++ b/gcc/config/riscv/riscv.cc @@ -3582,7 +3582,8 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx cons, rtx alt) enforce that so that we don't strip away a sign_extension thinking it is unnecessary. We might consider using riscv_extend_operands if they are not already properly extended. */ - if (GET_MODE (op0) != word_mode || GET_MODE (op1) != word_mode) + if ((GET_MODE (op0) != word_mode && GET_MODE (op0) != VOIDmode) + || (GET_MODE (op1) != word_mode && GET_MODE (op1) != VOIDmode)) return false; /* Canonicalize the comparison. It must be an equality comparison
This test is too aggressive. Constants have VOIDmode, so we need to let the through this phase of conditional move support. Fixes several missed conditional moves with the trunk. Committed to the trunk, Jeff commit 18c453f0e633abb9b317947b011ec6e07780fba8 Author: Jeff Law <jlaw@ventanamicro.com> Date: Mon Aug 7 14:34:40 2023 -0600 [committed][RISC-V]Don't reject constants in cmov condition This test is too aggressive. Constants have VOIDmode, so we need to let the through this phase of conditional move support. Fixes several missed conditional moves with the trunk. gcc/ * config/riscv/riscv.cc (riscv_expand_conditional_move): Allow VOIDmode operands to conditional before canonicalization.