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

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

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 214 bytes --]


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

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 1304 bytes --]

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.

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

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

[Vineet Gupta]

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

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

[Jeff Law]

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

RV64 Zicond ICE (was Re: [committed][RISC-V]Don't reject constants in cmov condition)

[Vineet Gupta]

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

Re: RV64 Zicond ICE (was Re: [committed][RISC-V]Don't reject constants in cmov condition)

[Jeff Law]

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

Re: RV64 Zicond ICE (was Re: [committed][RISC-V]Don't reject constants in cmov condition)

[Jeff Law]

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