Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Jeff Law via Gcc-patches]

On 8/3/23 08:56, Kito Cheng wrote:

That'll be the first thing to look at.  THe costing change was supposed
only affect if-then-else constructs, not sets in general.



If so, I think the most simple fix is adding more checks on the set
cost - only check the SET_SRC is if-then-else?

No, the simple fix is to just remove the errant part of the commit :-0
My tests aren't done, but that does seem to dramatically help.  Given it
wasn't supposed to go in as-is and it's causing major problems, I'll
probably just rip it out even though my testing isn't done.


OK, so I'm going to retreat from there, I've another lld issue that
needs to be fixed before the LLVM 17 release :)

Reversion of errant hunk has been pushed.  Sorry for the problems folks.

Had I known it was going to have this kind of fallout, I would have 
slammed a coke and fixed it last night before doing to sleep :-0



And yes, focusing on the lld issue seems wise given what I'm hearing in 
the LLVM meeting.


Jeff

Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Kito Cheng via Gcc-patches]

> >> That'll be the first thing to look at.  THe costing change was supposed
> >> only affect if-then-else constructs, not sets in general.
> >
> >
> > If so, I think the most simple fix is adding more checks on the set
> > cost - only check the SET_SRC is if-then-else?
> No, the simple fix is to just remove the errant part of the commit :-0
> My tests aren't done, but that does seem to dramatically help.  Given it
> wasn't supposed to go in as-is and it's causing major problems, I'll
> probably just rip it out even though my testing isn't done.

OK, so I'm going to retreat from there, I've another lld issue that
needs to be fixed before the LLVM 17 release :)

>
> >
> > Let me run the regression to see if that works - although the current
> > vsetvli cost is too high (4x~5x), but I think it should be fixed later
> > with a more complete expermantal.
> Exactly.  I think we need to do a full audit of the costing paths.  I've
> been slowly devising a way to do that and I'll probably give it to
> Raphael or Jivan once I've fleshed it out a bit more in my head.
>
> The goal is to make sure the costs are sensible and consistent across
> the different interfaces.  A cost failure is actually a bit hard to find
> because all that happens is you get the wrong set of transformations --
> but the code still works correctly, it's just not as efficient as it
> should be.  It doesn't have to be perfect, but we've clearly got a problem.
>
> WRT vsetvli costing.  That may ultimately be something that's uarch
> dependent.  We're working on the assumption that vsetvlis are common in
> the code stream and they need to be very efficient from the hardware
> standpoint (think as cheap or cheaper than any simple ALU instruction).
> I probably can't say what we're doing, but I bet it wouldn't be a
> surprise to others doing a high performance V implementation.

Yeah, it should be cheap, but might be expensive on some HW implementation,
anyway our cost model really needs to be tidy up at some point...:P

> jeff

Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Jeff Law via Gcc-patches]

On 8/3/23 08:31, Kito Cheng wrote:

I am working on that, it seems the cost of vsetvli instruction become 0
due to this change, then loop invariant motion won't hoist vsetvli longer.

I haven't looked yet (generating baseline rvv.exp data right now).  But
before I went to bed last night I was worried that a change snuck
through that shouldn't have (changing the toplevel INSN/SET cost
handling -- that wasn't supposed to be in the commit).  I was too tired
to verify and correct without possibly mucking it up further.

That'll be the first thing to look at.  THe costing change was supposed
only affect if-then-else constructs, not sets in general.



If so, I think the most simple fix is adding more checks on the set
cost - only check the SET_SRC is if-then-else?
No, the simple fix is to just remove the errant part of the commit :-0 
My tests aren't done, but that does seem to dramatically help.  Given it 
wasn't supposed to go in as-is and it's causing major problems, I'll 
probably just rip it out even though my testing isn't done.





Let me run the regression to see if that works - although the current
vsetvli cost is too high (4x~5x), but I think it should be fixed later
with a more complete expermantal.
Exactly.  I think we need to do a full audit of the costing paths.  I've 
been slowly devising a way to do that and I'll probably give it to 
Raphael or Jivan once I've fleshed it out a bit more in my head.


The goal is to make sure the costs are sensible and consistent across 
the different interfaces.  A cost failure is actually a bit hard to find 
because all that happens is you get the wrong set of transformations -- 
but the code still works correctly, it's just not as efficient as it 
should be.  It doesn't have to be perfect, but we've clearly got a problem.


WRT vsetvli costing.  That may ultimately be something that's uarch 
dependent.  We're working on the assumption that vsetvlis are common in 
the code stream and they need to be very efficient from the hardware 
standpoint (think as cheap or cheaper than any simple ALU instruction). 
I probably can't say what we're doing, but I bet it wouldn't be a 
surprise to others doing a high performance V implementation.


jeff

Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Kito Cheng via Gcc-patches]

> > I am working on that, it seems the cost of vsetvli instruction become 0
> > due to this change, then loop invariant motion won't hoist vsetvli longer.
> I haven't looked yet (generating baseline rvv.exp data right now).  But
> before I went to bed last night I was worried that a change snuck
> through that shouldn't have (changing the toplevel INSN/SET cost
> handling -- that wasn't supposed to be in the commit).  I was too tired
> to verify and correct without possibly mucking it up further.
>
> That'll be the first thing to look at.  THe costing change was supposed
> only affect if-then-else constructs, not sets in general.


If so, I think the most simple fix is adding more checks on the set
cost - only check the SET_SRC is if-then-else?

Let me run the regression to see if that works - although the current
vsetvli cost is too high (4x~5x), but I think it should be fixed later
with a more complete expermantal.

Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Jeff Law via Gcc-patches]

On 8/3/23 07:56, Kito Cheng wrote:
I am working on that, it seems the cost of vsetvli instruction become 0 
due to this change, then loop invariant motion won't hoist vsetvli longer.
I haven't looked yet (generating baseline rvv.exp data right now).  But 
before I went to bed last night I was worried that a change snuck 
through that shouldn't have (changing the toplevel INSN/SET cost 
handling -- that wasn't supposed to be in the commit).  I was too tired 
to verify and correct without possibly mucking it up further.


That'll be the first thing to look at.  THe costing change was supposed 
only affect if-then-else constructs, not sets in general.


Jeff

Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Kito Cheng via Gcc-patches]

I am working on that, it seems the cost of vsetvli instruction become 0 due
to this change, then loop invariant motion won't hoist vsetvli longer.

Jeff Law  於 2023年8月3日 週四 21:49 寫道：

>
>
> On 8/3/23 03:27, juzhe.zh...@rivai.ai wrote:
> >
> https://github.com/gcc-mirror/gcc/commit/e15d0b6680d10d7666195e9db65581364ad5e5df
> <
> https://github.com/gcc-mirror/gcc/commit/e15d0b6680d10d7666195e9db65581364ad5e5df
> >
> >
> > This patch causes so many fails in the regression:
> Mine.  I'll take care of it.  Probably something slipping through the
> expander that shouldn't.  I've been primarily focused on the execute.exp
> part of the suite to find code correctness issues with the original patch.
>
> jeff
>

Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Jeff Law via Gcc-patches]

On 8/3/23 03:27, juzhe.zh...@rivai.ai wrote:

https://github.com/gcc-mirror/gcc/commit/e15d0b6680d10d7666195e9db65581364ad5e5df 


This patch causes so many fails in the regression:
Mine.  I'll take care of it.  Probably something slipping through the 
expander that shouldn't.  I've been primarily focused on the execute.exp 
part of the suite to find code correctness issues with the original patch.


jeff

[PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[juzhe.zh...@rivai.ai]

https://github.com/gcc-mirror/gcc/commit/e15d0b6680d10d7666195e9db65581364ad5e5df
 

This patch causes so many fails in the regression:

FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-2.c   -O1   
scan-assembler-times vsetvli 3
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_back_prop-40.c   -O2   
scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*mf2,\\s*t[au],\\s*m[au]\\s+\\.L[0-9]+ 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-2.c   -O2   
scan-assembler-times vsetvli 3
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-2.c   -O2   
scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*m8,\\s*t[au],\\s*m[au]\\s+\\.L[0-9]+ 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-2.c   -O2   
scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*m8,\\s*t[au],\\s*m[au]\\s+j\\s+\\.L[0-9]+
 1
FAIL: gcc.target/riscv/rvv/vsetvl/avl_single-79.c   -Os   scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e32,\\s*mf2,\\s*tu,\\s*mu 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_back_prop-40.c   -O2 -flto 
-fno-use-linker-plugin -flto-partition=none   scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*mf2,\\s*t[au],\\s*m[au]\\s+\\.L[0-9]+ 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-2.c   -O2 -flto 
-fno-use-linker-plugin -flto-partition=none   scan-assembler-times vsetvli 3
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-2.c   -O2 -flto 
-fno-use-linker-plugin -flto-partition=none   scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*m8,\\s*t[au],\\s*m[au]\\s+\\.L[0-9]+ 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-2.c   -O2 -flto 
-fno-use-linker-plugin -flto-partition=none   scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*m8,\\s*t[au],\\s*m[au]\\s+j\\s+\\.L[0-9]+
 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_back_prop-40.c   -O2 -flto 
-fuse-linker-plugin -fno-fat-lto-objects   scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*mf2,\\s*t[au],\\s*m[au]\\s+\\.L[0-9]+ 1
FAIL: gcc.target/riscv/rvv/vsetvl/imm_loop_invariant-5.c   -Os   
scan-assembler-times 
\\.L[0-9]+\\:\\s+vle64\\.v\\s+v[0-9]+,\\s*0\\s*\\([a-x0-9]+\\) 8
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-2.c   -O2 -flto 
-fuse-linker-plugin -fno-fat-lto-objects   scan-assembler-times vsetvli 3
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-2.c   -O2 -flto 
-fuse-linker-plugin -fno-fat-lto-objects   scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*m8,\\s*t[au],\\s*m[au]\\s+\\.L[0-9]+ 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-2.c   -O2 -flto 
-fuse-linker-plugin -fno-fat-lto-objects   scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*m8,\\s*t[au],\\s*m[au]\\s+j\\s+\\.L[0-9]+
 1
FAIL: gcc.target/riscv/rvv/vsetvl/avl_multiple-10.c   -O1   
scan-assembler-times 
\\.L[0-9]+\\:\\s+add\\s+\\s*[a-x0-9]+,\\s*[a-x0-9]+,\\s*[a-x0-9]+\\s+vle8\\.v\\s+v[0-9]+,\\s*0\\s*\\([a-x0-9]+\\)
 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_back_prop-41.c   -O2   
scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*mf2,\\s*t[au],\\s*m[au] 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_back_prop-41.c   -O2   
scan-assembler-times vsetvli 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-3.c   -O2   
scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*mf8,\\s*t[au],\\s*m[au] 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-3.c   -O2   
scan-assembler-times vsetvli 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_back_prop-41.c   -O2 -flto 
-fno-use-linker-plugin -flto-partition=none   scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*mf2,\\s*t[au],\\s*m[au] 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_back_prop-41.c   -O2 -flto 
-fno-use-linker-plugin -flto-partition=none   scan-assembler-times vsetvli 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-3.c   -O2 -flto 
-fno-use-linker-plugin -flto-partition=none   scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*mf8,\\s*t[au],\\s*m[au] 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-3.c   -O2 -flto 
-fno-use-linker-plugin -flto-partition=none   scan-assembler-times vsetvli 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_back_prop-41.c   -O2 -flto 
-fuse-linker-plugin -fno-fat-lto-objects   scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*mf2,\\s*t[au],\\s*m[au] 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_back_prop-41.c   -O2 -flto 
-fuse-linker-plugin -fno-fat-lto-objects   scan-assembler-times vsetvli 1
FAIL: gcc.target/riscv/rvv/vsetvl/vsetvlmax-2.c   -Os   scan-assembler-times 
vsetvli 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-3.c   -O2 -flto 
-fuse-linker-plugin -fno-fat-lto-objects   scan-assembler-times 
vsetvli\\s+[a-x0-9]+,\\s*zero,\\s*e8,\\s*mf8,\\s*t[au],\\s*m[au] 1
FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_conflict-3.c   -O2 -flto 
-fuse-linker-plugin -fno-fat-lto-objects   scan-assembler-times vsetvli 1
FAIL: gcc.target/riscv/rvv/vsetvl/imm_loop_invariant-6.c   -Os   
scan-assembler-times 
\\.L[0-9]+\\:\\s+vle64\\.v\\s+v[0-9]+,\\s*0\\s*\\([a-x0-9]+\\) 

Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Jeff Law via Gcc-patches]

On 7/29/23 03:14, Xiao Zeng wrote:



1 Thank you for Jeff's code review comments. I have made the modifications
and submitted the V2-patch[3/5].
Yea.  I'm adjusting my tree based on those updates.  For testing I've 
actually got my compiler generating zicond by default and qemu allowing 
zicond by default.  I can then run the execute.exp tests which validate 
code correctness to a reasonable degree.





2 For the calculation method of cost, I hope to submit a separate patch[cost]
after the V2-patch[3/5] merged into master, which will focus on explaining
the reasons for calculating cost in the same way as in patch[4/5].
I think the costing problem is going to require its own little 
subproject.  GCC's approach to costing is a bit crazy with multiple APIs 
that behave differently and in some cases do some rather surprising 
things.  It's a long standing design flaw.


The point being that I think we'll probably move forward with the 
functional bits, perhaps initially without the basic functionality 
tests.  That allows folks to start utilizing the core functionality 
while we audit and likely adjust the risc-v cost hook implementation.





4. In V2-patch[3/5], Zicond's cost calculation is not involved, therefore, all 
test
cases are skipped with "- O0" and "- Os". I will remove the "- Os" constraint 
from
the test case in patch[cost].
We may need to avoid for -Og as well.  I've got that change here 
locally, but I wanted to go back and review that as well.


jeff

Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Jeff Law via Gcc-patches]

On 7/19/23 04:11, Xiao Zeng wrote:

This patch completes the recognition of Zicond when the select pattern
with condition eq or neq to 0 (using equality as an example), namely:

[ ... ]
I've committed the attached patch which implements a simple cost model 
for using Zicond to implement conditional moves.


I've changed it to give the proper cost (COSTS_N_INSNS (1) and removed 
the extraneous GET_CODE (object) tests adjusted the ChangeLog a bit and 
pushed it to the trunk.


Thanks!

Jeffcommit 5b501863ac7da57858fdd464dfb7a776143f22a2
Author: Xiao Zeng 
Date:   Wed Aug 2 00:17:12 2023 -0600

[PATCH 3/5] [RISC-V] Cost model for Zicond.

This patch implements a reasonable cost model for using Zicond to
implement conditional moves.  Essentially the Zicond insns are always
COSTS_N_INSNS (1).

Note there is still a problem with the costing model in general that
results in failure to if-convert as often as we should.  In simplest
terms the insn costing model sums the cost of the SET_SRC and the
cost of the SET_DEST.  Thus the conditional move is considered twice
as costly as it should be.  That will have to be addressed separately.

gcc/
* config/riscv/riscv.cc (riscv_rtx_costs): Add costing for
using Zicond to implement some conditional moves.

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 8c474503080..785e09c76ce 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -2518,6 +2518,20 @@ riscv_rtx_costs (rtx x, machine_mode mode, int 
outer_code, int opno ATTRIBUTE_UN
  *total = COSTS_N_INSNS (1);
  return true;
}
+  else if (TARGET_ZICOND
+  && outer_code == SET
+  && ((GET_CODE (XEXP (x, 1)) == REG
+   && XEXP (x, 2) == CONST0_RTX (GET_MODE (XEXP (x, 1
+  || (GET_CODE (XEXP (x, 2)) == REG
+  && XEXP (x, 1) == CONST0_RTX (GET_MODE (XEXP (x, 2
+  || (GET_CODE (XEXP (x, 1)) == REG
+  && rtx_equal_p (XEXP (x, 1), XEXP (XEXP (x, 0), 0)))
+  || (GET_CODE (XEXP (x, 1)) == REG
+  && rtx_equal_p (XEXP (x, 2), XEXP (XEXP (x, 0), 0)
+   {
+ *total = COSTS_N_INSNS (1);
+ return true;
+   }
   else if (LABEL_REF_P (XEXP (x, 1)) && XEXP (x, 2) == pc_rtx)
{
  if (equality_operator (XEXP (x, 0), mode)

Re: Re: [PATCH V2] [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Xiao Zeng]

On Tue, Aug 01, 2023 at 02:06:00 PM Jeff Law  wrote:
>
>
>
>On 7/29/23 03:13, Xiao Zeng wrote:
>> This patch recognizes Zicond patterns when the select pattern
>> with condition eq or neq to 0 (using eq as an example), namely:
>>
>> 1 rd = (rs2 == 0) ? non-imm : 0
>> 2 rd = (rs2 == 0) ? non-imm : non-imm
>> 3 rd = (rs2 == 0) ? reg : non-imm
>> 4 rd = (rs2 == 0) ? reg : reg
>>
>> gcc/ChangeLog:
>>
>>  * config/riscv/riscv.cc (riscv_expand_conditional_move): Recognize
>>  Zicond patterns
>>  * config/riscv/riscv.md: Recognize Zicond patterns through 
>>movcc
>>
>> gcc/testsuite/ChangeLog:
>>
>>  * gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c: New 
>>test.
>>  * gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c: New 
>>test.
>>  * gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c: New 
>>test.
>>  * gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c: New 
>>test.
>> ---
>>   gcc/config/riscv/riscv.cc | 144 ++
>>   gcc/config/riscv/riscv.md |   4 +-
>>   .../zicond-primitiveSemantics_return_0_imm.c  |  65 
>>   ...zicond-primitiveSemantics_return_imm_imm.c |  73 +
>>   ...zicond-primitiveSemantics_return_imm_reg.c |  65 
>>   ...zicond-primitiveSemantics_return_reg_reg.c |  65 
>>   6 files changed, 414 insertions(+), 2 deletions(-)
>>   create mode 100644 
>>gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c
>>   create mode 100644 
>>gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c
>>   create mode 100644 
>>gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c
>>   create mode 100644 
>>gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c
>>
>> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> index 941ea25e1f2..6ac39f63dd7 100644
>> --- a/gcc/config/riscv/riscv.cc
>> +++ b/gcc/config/riscv/riscv.cc
>> @@ -3516,6 +3516,150 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx 
>> cons, rtx alt)
>>     cond, cons, alt)));
>> return true;
>>   }
>> +  else if (TARGET_ZICOND
>> +   && (code == EQ || code == NE)
>> +   && GET_MODE_CLASS (mode) == MODE_INT)
>> +    {
>> +  need_eq_ne_p = true;
>> +  /* 0 + imm  */
>> +  if (GET_CODE (cons) == CONST_INT && cons == const0_rtx
>> +  && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
>A couple nits.  Rather than test the GET_CODE (object) == CONST_INT,
>instead use CONST_INT_P (object). 
fixed

>
>Rather than using const0_rtx, use CONST0_RTX (mode).  That makes it more
>general. 
fixed

>
>
>
>> +    {
>> +  riscv_emit_int_compare (, , , need_eq_ne_p);
>Might as well use "true" rather than "need_eq_ne_p" here and for the
>other calls in your new code.
> 
fixed

>> +  /* imm + imm  */
>> +  else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
>> +   && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
>So same comments on using CONST_INT_P and CONST0_RTX 
fixed

>> +    {
>> +  riscv_emit_int_compare (, , , need_eq_ne_p);
>> +  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> +  rtx reg = gen_reg_rtx (mode);
>> +  rtx temp = GEN_INT (INTVAL (alt) - INTVAL (cons));
>> +  emit_insn (gen_rtx_SET (reg, temp));
>Use force_reg here rather than directly emitting the insn to initialize
>"reg".  What you're doing works when the difference is small but will
>not work when the difference does not fit into a signed 12bit value. 
fixed

>
>> +  /* imm + reg  */
>> +  else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
>> +   && GET_CODE (alt) == REG)
>Same comments about CONST_INT_P and CONST0_RTX.  And instead of using
>GET_CODE (object) == REG, use REG_P (object).
>
>
>> +    {
>> +  /* Optimize for register value of 0.  */
>> +  if (op0 == alt && op1 == const0_rtx)
>> +    {
>> +  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> +  cons = force_reg (mode, cons);
>> +  emit_insn (gen_rtx_SET (dest, gen_rtx_IF_THEN_ELSE (mode, 
>> cond,
>> +  cons, 
>> alt)));
>> +  return true;
>> +    }
>Isn't this only valid for NE?
Here is what I didn't express clearly, please see the following patterns in 
zicond.md:

(define_insn "*czero.eqz..opt2"
  [(set (match_operand:GPR 0 "register_operand"                   "=r")
        (if_then_else:GPR (eq (match_operand:ANYI 1 "register_operand" "r")
                              (const_int 0))
                          (match_operand:GPR 2 "register_operand" "r")
                          (match_operand:GPR 3 "register_operand" "1")))]
  "TARGET_ZICOND && rtx_equal_p (operands[1],  operands[3])"
  "czero.nez\t%0,%2,%1"
)

Re: [PATCH V2] [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Jeff Law via Gcc-patches]

On 7/29/23 03:13, Xiao Zeng wrote:

This patch recognizes Zicond patterns when the select pattern
with condition eq or neq to 0 (using eq as an example), namely:

1 rd = (rs2 == 0) ? non-imm : 0
2 rd = (rs2 == 0) ? non-imm : non-imm
3 rd = (rs2 == 0) ? reg : non-imm
4 rd = (rs2 == 0) ? reg : reg

gcc/ChangeLog:

 * config/riscv/riscv.cc (riscv_expand_conditional_move): Recognize
 Zicond patterns
 * config/riscv/riscv.md: Recognize Zicond patterns through movcc

gcc/testsuite/ChangeLog:

 * gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c: New test.
 * gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c: New 
test.
 * gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c: New 
test.
 * gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c: New 
test.
---
  gcc/config/riscv/riscv.cc | 144 ++
  gcc/config/riscv/riscv.md |   4 +-
  .../zicond-primitiveSemantics_return_0_imm.c  |  65 
  ...zicond-primitiveSemantics_return_imm_imm.c |  73 +
  ...zicond-primitiveSemantics_return_imm_reg.c |  65 
  ...zicond-primitiveSemantics_return_reg_reg.c |  65 
  6 files changed, 414 insertions(+), 2 deletions(-)
  create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c
  create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c
  create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c
  create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 941ea25e1f2..6ac39f63dd7 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -3516,6 +3516,150 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx 
cons, rtx alt)
  cond, cons, alt)));
return true;
  }
+  else if (TARGET_ZICOND
+   && (code == EQ || code == NE)
+   && GET_MODE_CLASS (mode) == MODE_INT)
+{
+  need_eq_ne_p = true;
+  /* 0 + imm  */
+  if (GET_CODE (cons) == CONST_INT && cons == const0_rtx
+  && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
A couple nits.  Rather than test the GET_CODE (object) == CONST_INT, 
instead use CONST_INT_P (object).


Rather than using const0_rtx, use CONST0_RTX (mode).  That makes it more 
general.





+{
+  riscv_emit_int_compare (, , , need_eq_ne_p);
Might as well use "true" rather than "need_eq_ne_p" here and for the 
other calls in your new code.



+  /* imm + imm  */
+  else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
+   && GET_CODE (alt) == CONST_INT && alt != const0_rtx)

So same comments on using CONST_INT_P and CONST0_RTX

+{
+  riscv_emit_int_compare (, , , need_eq_ne_p);
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  rtx reg = gen_reg_rtx (mode);
+  rtx temp = GEN_INT (INTVAL (alt) - INTVAL (cons));
+  emit_insn (gen_rtx_SET (reg, temp));
Use force_reg here rather than directly emitting the insn to initialize 
"reg".  What you're doing works when the difference is small but will 
not work when the difference does not fit into a signed 12bit value.



+  /* imm + reg  */
+  else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
+   && GET_CODE (alt) == REG)
Same comments about CONST_INT_P and CONST0_RTX.  And instead of using 
GET_CODE (object) == REG, use REG_P (object).




+{
+  /* Optimize for register value of 0.  */
+  if (op0 == alt && op1 == const0_rtx)
+{
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  cons = force_reg (mode, cons);
+  emit_insn (gen_rtx_SET (dest, gen_rtx_IF_THEN_ELSE (mode, cond,
+  cons, alt)));
+  return true;
+}

Isn't this only valid for NE?  Also use CONST0_RTX in that test.



+  /* Handle the special situation of: -2048 == INTVAL (alt)
+ to avoid failure due to an unrecognized insn. Let the costing
+ model determine if the conditional move sequence is better
+ than the branching sequence.  */
+  if (-2048 == INTVAL (cons))
So instead of checking for explicit values, we have SMALL_OPERAND to do 
it for us.  Also note that for !SMALL_OPERAND we can just force the 
value into a register using "force_reg" and all the right things will 
happen.


So just add something like this to your original code:

  if (!SMALL_OPERAND (INTVAL (cons))
cons = force_reg (mode, cons);

That will result in CONS being either a simple integer constant (when it 
is suitable for addi) or a register (all other cases).  At that point 

Re: Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Xiao Zeng]

On Fri, Jul 28, 2023 at 11:09:00 PM  Jeff Law  wrote:
>
>
>
>On 7/25/23 11:55, Andreas Schwab wrote:
>> On Jul 19 2023, Xiao Zeng wrote:
>>
>>> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>>> index 38d8eb2fcf5..7e6b24bd232 100644
>>> --- a/gcc/config/riscv/riscv.cc
>>> +++ b/gcc/config/riscv/riscv.cc
>>> @@ -2448,6 +2448,17 @@ riscv_rtx_costs (rtx x, machine_mode mode, int 
>>> outer_code, int opno ATTRIBUTE_UN
>>>     *total = COSTS_N_INSNS (1);
>>>     return true;
>>>   }
>>> +  else if (TARGET_ZICOND && outer_code == SET &&
>>> +   ((GET_CODE (XEXP (x, 1)) == REG && XEXP (x, 2) == 
>>> const0_rtx) ||
>>> +   (GET_CODE (XEXP (x, 2)) == REG && XEXP (x, 1) == 
>>> const0_rtx) ||
>>> +   (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
>>> +    XEXP (x, 1) == XEXP (XEXP (x, 0), 0)) ||
>>> +   (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
>>> +    XEXP (x, 2) == XEXP (XEXP (x, 0), 0
>>
>> Line breaks before the operator, not after.
>Also note that && GET_CODE (XEXP (x, 2)) && that appears twice. 

This is an error that I will fix in patch[cost] and provide a detailed 
explanation.

>
>That just verifies the code isn't RTX_UNKNOWN which I suspect isn't what
>the author intended.  It probably needs to be adjusted for SUBREGs and
>the pointer equality issues with REGs after reload.
>
>I'll take care of these goofs since the costing ought to be able to move
>forward independently of the improvements Xiao made to generating
>conditional move sequences.
>
>Jeff 

After V2-patch[3/5] is accepted, a patch[cost] will be submitted to provide 
detailed
explanation of this issue. Of course, as Jeff mentioned, some issues will also 
be fixed.

Thanks
Xiao Zeng

Re: Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Xiao Zeng]

On Sat, Jul 29, 2023 at 04:59:00 AM  Jeff Law  wrote:
>
>
>
>On 7/19/23 04:11, Xiao Zeng wrote:
>
>> +  else if (TARGET_ZICOND
>> +   && (code == EQ || code == NE)
>> +   && GET_MODE_CLASS (mode) == MODE_INT)
>> +    {
>> +  need_eq_ne_p = true;
>> +  /* 0 + imm  */
>> +  if (GET_CODE (cons) == CONST_INT && cons == const0_rtx
>> +  && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
>> +    {
>> +  riscv_emit_int_compare (, , , need_eq_ne_p);
>> +  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> +  alt = force_reg (mode, alt);
>> +  emit_insn (gen_rtx_SET (dest,
>> +  gen_rtx_IF_THEN_ELSE (mode, cond,
>> +    cons, alt)));
>> +  return true;
>> +    }
>> +  /* imm + imm  */
>> +  else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
>> +   && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
>> +    {
>> +  riscv_emit_int_compare (, , , need_eq_ne_p);
>> +  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> +  alt = force_reg (mode, alt);
>> +  rtx temp1 = gen_reg_rtx (mode);
>> +  rtx temp2 = GEN_INT(-1 * INTVAL (cons));
>> +  riscv_emit_binary(PLUS, temp1, alt, temp2);
>So in this sequence you're just computing a constant since both ALT and
>CONS are constants.  It's better to just form the constant directly,
>then force that into a register because it'll make the costing more
>correct, particularly if the resulting constant needs more than one
>instruction to synthesize. 

Fixed

>
>And a nit.  There should always be a space between a function name and
>its argument list. 

Fixed

>
>
>
>> +  emit_insn (gen_rtx_SET (dest,
>> +  gen_rtx_IF_THEN_ELSE (mode, cond,
>> +    const0_rtx, alt)));
>> +  riscv_emit_binary(PLUS, dest, dest, cons);
>> +  return true;
>I don't see how this can be correct from a code generation standpoint.
>You compute ALT-CONS into TEMP1 earlier.  But you never use TEMP1 after
>that.  I think you meant to use TEMP1 instead of ALT as the false arm if
>the IF-THEN-ELSE you constructed. 

Fixed

>
>In general you should be using CONST0_RTX (mode) rather than const0_rtx.
> 

Fixed

>> +    }
>> +  /* imm + reg  */
>> +  else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
>> +   && GET_CODE (alt) == REG)
>> +    {
>> +  /* Optimize for register value of 0.  */
>> +  if (op0 == alt && op1 == const0_rtx)
>> +    {
>> +  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> +  cons = force_reg (mode, cons);
>> +  emit_insn (gen_rtx_SET (dest,
>> +  gen_rtx_IF_THEN_ELSE (mode, cond,
>> +    cons, alt)));
>> +  return true;
>> +    }
>> +  riscv_emit_int_compare (, , , need_eq_ne_p);
>> +  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> +  rtx temp1 = gen_reg_rtx (mode);
>> +  rtx temp2 = GEN_INT(-1 * INTVAL (cons));
>> +  riscv_emit_binary(PLUS, temp1, alt, temp2);
>Here you have to be careful if CONS is -2048.  You negate it resulting
>in +2048 which can't be used in an addi.  This will cause the entire
>sequence to fail due to an unrecognized insn.  It would be better to
>handle that scenario directly so the generated sequence is still valid.
>
>By generating recognizable code in that case we let the costing model
>determine if the conditional move sequence is better than the branching
>sequence. 

Thank you for pointing out this special situation, it has been fixed

>
>
>> +  emit_insn (gen_rtx_SET (dest,
>> +  gen_rtx_IF_THEN_ELSE (mode, cond,
>> +    const0_rtx, alt)));
>I think we have the same problem with the use of ALT here rather than
>TEMP1 that we had in the previous case. 

Fixed

>
>
>
>> +  /* reg + imm  */
>> +  else if (GET_CODE (cons) == REG
>> +   && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
>> +    {
>> +  /* Optimize for register value of 0.  */
>> +  if (op0 == cons && op1 == const0_rtx)
>> +    {
>> +  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> +  alt = force_reg (mode, alt);
>> +  emit_insn (gen_rtx_SET (dest,
>> +  gen_rtx_IF_THEN_ELSE (mode, cond,
>> +    cons, alt)));
>> +  return true;
>> +    }
>> +  riscv_emit_int_compare (, , , need_eq_ne_p);
>> +  rtx cond = gen_rtx_fmt_ee (code, GET_MODE 

[PATCH V2] [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Xiao Zeng]

This patch recognizes Zicond patterns when the select pattern
with condition eq or neq to 0 (using eq as an example), namely:

1 rd = (rs2 == 0) ? non-imm : 0
2 rd = (rs2 == 0) ? non-imm : non-imm
3 rd = (rs2 == 0) ? reg : non-imm
4 rd = (rs2 == 0) ? reg : reg

gcc/ChangeLog:

* config/riscv/riscv.cc (riscv_expand_conditional_move): Recognize
Zicond patterns
* config/riscv/riscv.md: Recognize Zicond patterns through movcc

gcc/testsuite/ChangeLog:

* gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c: New test.
* gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c: New test.
* gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c: New test.
* gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c: New test.
---
 gcc/config/riscv/riscv.cc | 144 ++
 gcc/config/riscv/riscv.md |   4 +-
 .../zicond-primitiveSemantics_return_0_imm.c  |  65 
 ...zicond-primitiveSemantics_return_imm_imm.c |  73 +
 ...zicond-primitiveSemantics_return_imm_reg.c |  65 
 ...zicond-primitiveSemantics_return_reg_reg.c |  65 
 6 files changed, 414 insertions(+), 2 deletions(-)
 create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c
 create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c
 create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c
 create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 941ea25e1f2..6ac39f63dd7 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -3516,6 +3516,150 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx 
cons, rtx alt)
  cond, cons, alt)));
   return true;
 }
+  else if (TARGET_ZICOND
+   && (code == EQ || code == NE)
+   && GET_MODE_CLASS (mode) == MODE_INT)
+{
+  need_eq_ne_p = true;
+  /* 0 + imm  */
+  if (GET_CODE (cons) == CONST_INT && cons == const0_rtx
+  && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
+{
+  riscv_emit_int_compare (, , , need_eq_ne_p);
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  alt = force_reg (mode, alt);
+  emit_insn (gen_rtx_SET (dest, gen_rtx_IF_THEN_ELSE (mode, cond,
+  cons, alt)));
+  return true;
+}
+  /* imm + imm  */
+  else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
+   && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
+{
+  riscv_emit_int_compare (, , , need_eq_ne_p);
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  rtx reg = gen_reg_rtx (mode);
+  rtx temp = GEN_INT (INTVAL (alt) - INTVAL (cons));
+  emit_insn (gen_rtx_SET (reg, temp));
+  emit_insn (gen_rtx_SET (dest, gen_rtx_IF_THEN_ELSE (mode, cond,
+  CONST0_RTX 
(mode),
+  reg)));
+  riscv_emit_binary (PLUS, dest, dest, cons);
+  return true;
+}
+  /* imm + reg  */
+  else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
+   && GET_CODE (alt) == REG)
+{
+  /* Optimize for register value of 0.  */
+  if (op0 == alt && op1 == const0_rtx)
+{
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  cons = force_reg (mode, cons);
+  emit_insn (gen_rtx_SET (dest, gen_rtx_IF_THEN_ELSE (mode, cond,
+  cons, alt)));
+  return true;
+}
+  /* Handle the special situation of: -2048 == INTVAL (alt)
+ to avoid failure due to an unrecognized insn. Let the costing
+ model determine if the conditional move sequence is better
+ than the branching sequence.  */
+  if (-2048 == INTVAL (cons))
+{
+  rtx reg = gen_reg_rtx (mode);
+  emit_insn (gen_rtx_SET (reg, cons));
+  return riscv_expand_conditional_move (dest, op, reg, alt);
+}
+  riscv_emit_int_compare (, , , need_eq_ne_p);
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  rtx temp = GEN_INT (-1 * INTVAL (cons));
+  riscv_emit_binary (PLUS, alt, alt, temp);
+  emit_insn (gen_rtx_SET (dest, gen_rtx_IF_THEN_ELSE (mode, cond,
+  CONST0_RTX 
(mode),
+  alt)));
+  riscv_emit_binary 

Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Jeff Law via Gcc-patches]

On 7/19/23 04:11, Xiao Zeng wrote:


+  else if (TARGET_ZICOND
+   && (code == EQ || code == NE)
+   && GET_MODE_CLASS (mode) == MODE_INT)
+{
+  need_eq_ne_p = true;
+  /* 0 + imm  */
+  if (GET_CODE (cons) == CONST_INT && cons == const0_rtx
+  && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
+{
+  riscv_emit_int_compare (, , , need_eq_ne_p);
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  alt = force_reg (mode, alt);
+  emit_insn (gen_rtx_SET (dest,
+  gen_rtx_IF_THEN_ELSE (mode, cond,
+cons, alt)));
+  return true;
+}
+  /* imm + imm  */
+  else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
+   && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
+{
+  riscv_emit_int_compare (, , , need_eq_ne_p);
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  alt = force_reg (mode, alt);
+  rtx temp1 = gen_reg_rtx (mode);
+  rtx temp2 = GEN_INT(-1 * INTVAL (cons));
+  riscv_emit_binary(PLUS, temp1, alt, temp2);
So in this sequence you're just computing a constant since both ALT and 
CONS are constants.  It's better to just form the constant directly, 
then force that into a register because it'll make the costing more 
correct, particularly if the resulting constant needs more than one 
instruction to synthesize.


And a nit.  There should always be a space between a function name and 
its argument list.





+  emit_insn (gen_rtx_SET (dest,
+  gen_rtx_IF_THEN_ELSE (mode, cond,
+const0_rtx, alt)));
+  riscv_emit_binary(PLUS, dest, dest, cons);
+  return true;
I don't see how this can be correct from a code generation standpoint. 
You compute ALT-CONS into TEMP1 earlier.  But you never use TEMP1 after 
that.  I think you meant to use TEMP1 instead of ALT as the false arm if 
the IF-THEN-ELSE you constructed.


In general you should be using CONST0_RTX (mode) rather than const0_rtx.


+}
+  /* imm + reg  */
+  else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
+   && GET_CODE (alt) == REG)
+{
+  /* Optimize for register value of 0.  */
+  if (op0 == alt && op1 == const0_rtx)
+{
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  cons = force_reg (mode, cons);
+  emit_insn (gen_rtx_SET (dest,
+  gen_rtx_IF_THEN_ELSE (mode, cond,
+cons, alt)));
+  return true;
+}
+  riscv_emit_int_compare (, , , need_eq_ne_p);
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  rtx temp1 = gen_reg_rtx (mode);
+  rtx temp2 = GEN_INT(-1 * INTVAL (cons));
+  riscv_emit_binary(PLUS, temp1, alt, temp2);
Here you have to be careful if CONS is -2048.  You negate it resulting 
in +2048 which can't be used in an addi.  This will cause the entire 
sequence to fail due to an unrecognized insn.  It would be better to 
handle that scenario directly so the generated sequence is still valid.


By generating recognizable code in that case we let the costing model 
determine if the conditional move sequence is better than the branching 
sequence.




+  emit_insn (gen_rtx_SET (dest,
+  gen_rtx_IF_THEN_ELSE (mode, cond,
+const0_rtx, alt)));
I think we have the same problem with the use of ALT here rather than 
TEMP1 that we had in the previous case.





+  /* reg + imm  */
+  else if (GET_CODE (cons) == REG
+   && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
+{
+  /* Optimize for register value of 0.  */
+  if (op0 == cons && op1 == const0_rtx)
+{
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  alt = force_reg (mode, alt);
+  emit_insn (gen_rtx_SET (dest,
+  gen_rtx_IF_THEN_ELSE (mode, cond,
+cons, alt)));
+  return true;
+}
+  riscv_emit_int_compare (, , , need_eq_ne_p);
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  rtx temp1 = gen_reg_rtx (mode);
+  rtx temp2 = GEN_INT(-1 * INTVAL (alt));
+  riscv_emit_binary(PLUS, temp1, cons, temp2);
+  emit_insn (gen_rtx_SET (dest,
+  gen_rtx_IF_THEN_ELSE (mode, cond,
+temp1, const0_rtx)));
+  riscv_emit_binary(PLUS, dest, 

Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Jeff Law via Gcc-patches]

On 7/25/23 11:55, Andreas Schwab wrote:

On Jul 19 2023, Xiao Zeng wrote:


diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 38d8eb2fcf5..7e6b24bd232 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -2448,6 +2448,17 @@ riscv_rtx_costs (rtx x, machine_mode mode, int 
outer_code, int opno ATTRIBUTE_UN
  *total = COSTS_N_INSNS (1);
  return true;
}
+  else if (TARGET_ZICOND && outer_code == SET &&
+   ((GET_CODE (XEXP (x, 1)) == REG && XEXP (x, 2) == const0_rtx) ||
+   (GET_CODE (XEXP (x, 2)) == REG && XEXP (x, 1) == const0_rtx) ||
+   (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
+XEXP (x, 1) == XEXP (XEXP (x, 0), 0)) ||
+   (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
+XEXP (x, 2) == XEXP (XEXP (x, 0), 0


Line breaks before the operator, not after.

Also note that && GET_CODE (XEXP (x, 2)) && that appears twice.

That just verifies the code isn't RTX_UNKNOWN which I suspect isn't what 
the author intended.  It probably needs to be adjusted for SUBREGs and 
the pointer equality issues with REGs after reload.


I'll take care of these goofs since the costing ought to be able to move 
forward independently of the improvements Xiao made to generating 
conditional move sequences.


Jeff

Re: Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Xiao Zeng]

On Wed, Jul 26, 2023 at 01:55:00 AM Andreas Schwab  
wrote:
>
>On Jul 19 2023, Xiao Zeng wrote:
>
>> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> index 38d8eb2fcf5..7e6b24bd232 100644
>> --- a/gcc/config/riscv/riscv.cc
>> +++ b/gcc/config/riscv/riscv.cc
>> @@ -2448,6 +2448,17 @@ riscv_rtx_costs (rtx x, machine_mode mode, int 
>> outer_code, int opno ATTRIBUTE_UN
>>    *total = COSTS_N_INSNS (1);
>>    return true;
>>  }
>> +  else if (TARGET_ZICOND && outer_code == SET &&
>> +   ((GET_CODE (XEXP (x, 1)) == REG && XEXP (x, 2) == 
>> const0_rtx) ||
>> +   (GET_CODE (XEXP (x, 2)) == REG && XEXP (x, 1) == const0_rtx) 
>> ||
>> +   (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
>> +    XEXP (x, 1) == XEXP (XEXP (x, 0), 0)) ||
>> +   (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
>> +    XEXP (x, 2) == XEXP (XEXP (x, 0), 0
>
>Line breaks before the operator, not after.
>
>--
>Andreas Schwab, sch...@linux-m68k.org
>GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
>"And now for something completely different." 

Thank you for pointing out the code format issue. I will fix it in the future 
patch.

Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Andreas Schwab]

On Jul 19 2023, Xiao Zeng wrote:

> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 38d8eb2fcf5..7e6b24bd232 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -2448,6 +2448,17 @@ riscv_rtx_costs (rtx x, machine_mode mode, int 
> outer_code, int opno ATTRIBUTE_UN
> *total = COSTS_N_INSNS (1);
> return true;
>   }
> +  else if (TARGET_ZICOND && outer_code == SET &&
> +   ((GET_CODE (XEXP (x, 1)) == REG && XEXP (x, 2) == const0_rtx) 
> ||
> +   (GET_CODE (XEXP (x, 2)) == REG && XEXP (x, 1) == const0_rtx) 
> ||
> +   (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
> +XEXP (x, 1) == XEXP (XEXP (x, 0), 0)) ||
> +   (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
> +XEXP (x, 2) == XEXP (XEXP (x, 0), 0

Line breaks before the operator, not after.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Jeff Law via Gcc-patches]

On 7/19/23 04:11, Xiao Zeng wrote:

This patch completes the recognition of Zicond when the select pattern
with condition eq or neq to 0 (using equality as an example), namely:

1 rd = (rs2 == 0) ? non-imm : 0
2 rd = (rs2 == 0) ? non-imm : non-imm
3 rd = (rs2 == 0) ? reg : non-imm
4 rd = (rs2 == 0) ? reg : reg

gcc/ChangeLog:

 * config/riscv/riscv.cc (riscv_rtx_costs): IF_THEN_ELSE costs in 
Zicond.
 (riscv_expand_conditional_move): Recognize Zicond.
 * config/riscv/riscv.md: Zicond patterns.

gcc/testsuite/ChangeLog:

* gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c: New test.
* gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c: New test.
* gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c: New test.
* gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c: New test.
---
  gcc/config/riscv/riscv.cc | 125 ++
  gcc/config/riscv/riscv.md |   2 +-
  .../zicond-primitiveSemantics_return_0_imm.c  |  65 +
  ...zicond-primitiveSemantics_return_imm_imm.c |  73 ++
  ...zicond-primitiveSemantics_return_imm_reg.c |  65 +
  ...zicond-primitiveSemantics_return_reg_reg.c |  65 +
  6 files changed, 394 insertions(+), 1 deletion(-)
  create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c
  create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c
  create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c
  create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 38d8eb2fcf5..7e6b24bd232 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -2448,6 +2448,17 @@ riscv_rtx_costs (rtx x, machine_mode mode, int 
outer_code, int opno ATTRIBUTE_UN
  *total = COSTS_N_INSNS (1);
  return true;
}
+  else if (TARGET_ZICOND && outer_code == SET &&
+   ((GET_CODE (XEXP (x, 1)) == REG && XEXP (x, 2) == const0_rtx) ||
+   (GET_CODE (XEXP (x, 2)) == REG && XEXP (x, 1) == const0_rtx) ||
+   (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
+XEXP (x, 1) == XEXP (XEXP (x, 0), 0)) ||
+   (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
+XEXP (x, 2) == XEXP (XEXP (x, 0), 0
+{
+  *total = 0;
+  return true;
+}

So why *total = 0.  I would have expected *total = COSTS_N_INSNS (1).


I'm not entirely sure the changes to riscv_expand_conditional_move are 
desirable -- these are likely better done in the generic if-conversion 
pass.




diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index 6b8c2e8e268..b4147c7a79c 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -2484,7 +2484,7 @@
(if_then_else:GPR (match_operand 1 "comparison_operator")
  (match_operand:GPR 2 "reg_or_0_operand")
  (match_operand:GPR 3 "sfb_alu_operand")))]
-  "TARGET_SFB_ALU || TARGET_XTHEADCONDMOV"
+  "TARGET_SFB_ALU || TARGET_XTHEADCONDMOV || TARGET_ZICOND"
  {
if (riscv_expand_conditional_move (operands[0], operands[1],
 operands[2], operands[3]))
We had to do more than just slap on a TARGET_ZICOND.  I'm a bit 
surprised this worked as-is.  Though we also have bits to handle 
conditions other than eq/ne by first emitting an sCC style insn which 
might be adding complication or cases you hadn't encountered.



Jeff

[PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Xiao Zeng]

This patch completes the recognition of Zicond when the select pattern
with condition eq or neq to 0 (using equality as an example), namely:

1 rd = (rs2 == 0) ? non-imm : 0
2 rd = (rs2 == 0) ? non-imm : non-imm
3 rd = (rs2 == 0) ? reg : non-imm
4 rd = (rs2 == 0) ? reg : reg

gcc/ChangeLog:

* config/riscv/riscv.cc (riscv_rtx_costs): IF_THEN_ELSE costs in Zicond.
(riscv_expand_conditional_move): Recognize Zicond.
* config/riscv/riscv.md: Zicond patterns.

gcc/testsuite/ChangeLog:

* gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c: New test.
* gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c: New test.
* gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c: New test.
* gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c: New test.
---
 gcc/config/riscv/riscv.cc | 125 ++
 gcc/config/riscv/riscv.md |   2 +-
 .../zicond-primitiveSemantics_return_0_imm.c  |  65 +
 ...zicond-primitiveSemantics_return_imm_imm.c |  73 ++
 ...zicond-primitiveSemantics_return_imm_reg.c |  65 +
 ...zicond-primitiveSemantics_return_reg_reg.c |  65 +
 6 files changed, 394 insertions(+), 1 deletion(-)
 create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c
 create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c
 create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c
 create mode 100644 
gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 38d8eb2fcf5..7e6b24bd232 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -2448,6 +2448,17 @@ riscv_rtx_costs (rtx x, machine_mode mode, int 
outer_code, int opno ATTRIBUTE_UN
  *total = COSTS_N_INSNS (1);
  return true;
}
+  else if (TARGET_ZICOND && outer_code == SET &&
+   ((GET_CODE (XEXP (x, 1)) == REG && XEXP (x, 2) == const0_rtx) ||
+   (GET_CODE (XEXP (x, 2)) == REG && XEXP (x, 1) == const0_rtx) ||
+   (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
+XEXP (x, 1) == XEXP (XEXP (x, 0), 0)) ||
+   (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
+XEXP (x, 2) == XEXP (XEXP (x, 0), 0
+{
+  *total = 0;
+  return true;
+}
   else if (LABEL_REF_P (XEXP (x, 1)) && XEXP (x, 2) == pc_rtx)
{
  if (equality_operator (XEXP (x, 0), mode)
@@ -3501,6 +3512,120 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx 
cons, rtx alt)
  cond, cons, alt)));
   return true;
 }
+  else if (TARGET_ZICOND
+   && (code == EQ || code == NE)
+   && GET_MODE_CLASS (mode) == MODE_INT)
+{
+  need_eq_ne_p = true;
+  /* 0 + imm  */
+  if (GET_CODE (cons) == CONST_INT && cons == const0_rtx
+  && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
+{
+  riscv_emit_int_compare (, , , need_eq_ne_p);
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  alt = force_reg (mode, alt);
+  emit_insn (gen_rtx_SET (dest,
+  gen_rtx_IF_THEN_ELSE (mode, cond,
+cons, alt)));
+  return true;
+}
+  /* imm + imm  */
+  else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
+   && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
+{
+  riscv_emit_int_compare (, , , need_eq_ne_p);
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  alt = force_reg (mode, alt);
+  rtx temp1 = gen_reg_rtx (mode);
+  rtx temp2 = GEN_INT(-1 * INTVAL (cons));
+  riscv_emit_binary(PLUS, temp1, alt, temp2);
+  emit_insn (gen_rtx_SET (dest,
+  gen_rtx_IF_THEN_ELSE (mode, cond,
+const0_rtx, alt)));
+  riscv_emit_binary(PLUS, dest, dest, cons);
+  return true;
+}
+  /* imm + reg  */
+  else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
+   && GET_CODE (alt) == REG)
+{
+  /* Optimize for register value of 0.  */
+  if (op0 == alt && op1 == const0_rtx)
+{
+  rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
+  cons = force_reg (mode, cons);
+  emit_insn (gen_rtx_SET (dest,
+  gen_rtx_IF_THEN_ELSE (mode, cond,
+cons, alt)));
+  return true;
+}
+  riscv_emit_int_compare (, , ,