Re: Re: [PATCH 0/5] Recognize Zicond extension

["Xiao Zeng" <zengxiao@eswincomputing.com>]

On Wed, Jul 26, 2023 at 01:51:00 AM  Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
>On 7/19/23 04:11, Xiao Zeng wrote:
>> Hi all RISC-V folks:
>>
>> This series of patches completes support for the riscv architecture's
>> Zicond standard extension instruction set.
>>
>> Currently, Zicond is in a frozen state.
>>
>> See the Zicond specification for details:
>> https://github.com/riscv/riscv-zicond/releases/download/v1.0-rc2/riscv-zicond-v1.0-rc2.pdf
>>
>> Prior to this, other community members have also done related work, as shown in:
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611767.html
>> https://sourceware.org/pipermail/binutils/2023-January/125773.html
>>
>> Xiao Zeng (5):
>>    [RISC-V] Recognize Zicond extension
>>    [RISC-V] Generate Zicond instruction for basic semantics
>>    [RISC-V] Generate Zicond instruction for select pattern with condition
>>      eq or neq to 0
>>    [RISC-V] Generate Zicond instruction for select pattern with condition
>>      eq or neq to non-zero
>>    [RISC-V] Generate Zicond instruction for conditional execution
>[ ... ]
>So what I'm thinking for the overall kit is to stage it in a bit
>differently given we have some bits which clearly can go forward as-is
>or with very minor changes and others that are going to need some
>iteration/refinement.
>
>So I'm going to suggest a few changes so that bits which are non
>controversial can move forward immediately.
>
>1/5 looked fine as-is.
>
>I would split 2/5.  The first two patterns you added are
>non-controversial and could go in immediately.  The other 4 patterns
>(which require some operand matching) will likely need at least one
>round of iteration and should be a distinct patch.
>
>
>I would split 3/5 as well.  3a would be the costing which I think just
>needs to use COSTS_N_INSNS (1) rather than 0 for the cost of a
>conditional move and could then move forward immediately.  The bits to
>wire everything up into the conditional move pattern would be a distinct
>patch.  We did something similar internally in Ventana and I'd like to
>take the time to make sure the issues we ran into are addressed in your
>version then do an evaluation of the two approaches.
>
>I think patch 4 is probably going to need some work too.  I *think* what
>we did internally at Ventana will work better (utilizing scc for a
>non-trivial condition).
>
>Let's defer patch #5 initially as well.  It's going to get tangled up in
>a whole bunch of changes I think we need to make to ifcvt.cc.
>
>The point being that with the bits from #1, #2 and #3 we can get some
>initial support in immediately.  eswincomputing and ventana can both
>reduce our divergence from the trunk and work together on the rest of
>the bits.
>
>Does that work for you?
>
>jeff 

1 Thanks Jeff for your code review feedback.

2. According to your opinions, I have modified the code, but out of caution
for upstream, I conducted a complete regression tests on patch V2, which took
some time. I was unable to reply to emails and upload patch V2 in a timely manner.

3 After you and other maintainers made minor modifications to my patch[1/5] 
and patch[2/5], it has been merged into the master, so I will no longer upload patch V2.

4 patch[1/5] and patch[2/5], which have been merged into the master, have only
completed basic support for Zicond, and further optimization work needs to be
completed. These further optimization reactions are reflected in my patch[3/5]
patch[4/5] and patch[5/5].

5 As you mentioned in your previous email https://gcc.gnu.org/pipermail/gcc-patches/2023-July/625427.html
"eswincomputing and ventana can both reduce our divergence from the trunk
and work together on the rest of the bits...". I will reorganize patch[3/5] patch[4/5]
and patch[5/5], provide more detailed explanations, and submit them as an alternative
solution for further optimization of Zicond.

Does that work for you?

Xiao Zeng