diff mbox series

[committed,RISC-V] Don't reject constants in cmov condition

Message ID f913b618-f708-2018-898b-57a8b84bb07f@ventanamicro.com
State Unresolved
Headers
Series [committed,RISC-V] Don't reject constants in cmov condition |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jeff Law Aug. 7, 2023, 8:36 p.m. UTC 
  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.

Comments

Vineet Gupta Aug. 7, 2023, 11:42 p.m. UTC | #1 
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
Jeff Law Aug. 8, 2023, 2:27 p.m. UTC | #2 
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
Vineet Gupta Aug. 8, 2023, 5:21 p.m. UTC | #3 
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
Jeff Law Aug. 8, 2023, 5:46 p.m. UTC | #4 
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
Jeff Law Aug. 8, 2023, 9:41 p.m. UTC | #5 
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 mbox series

Patch

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