| Message ID | 20230719101156.21771-1-zengxiao@eswincomputing.com |
|---|---|
| Headers | show |
| Series | Recognize Zicond extension | expand |
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
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
On 7/27/23 02:43, Xiao Zeng wrote: > > 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. Sorry to have wasted your time -- zicond/xventanacondops has lingered for quite a while and I had a bit of free time yesterday. I felt it was most useful to try and move this stuff forward. > > 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. Agreed. > > 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]. Agreed. > > 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? I'm going to look at 3/5 today pretty closely. Exposing zicond to mov<node>cc is something we had implemented inside Ventana and I want to compare/contrast your work with ours. What I like about yours is it keeps all the logic in riscv.cc rather than scattering it across riscv.cc and riscv.md. What I like about the internal Ventana bits is its ability to support arbitrary comparisons by utilizing sCC if the original is not an eq/ne comparison. Ideally we'll be able to get the best of both. Jeff
On Thu, Jul 27, 2023 at 10:43:00 PM Jeff Law <jeffreyalaw@gmail.com> wrote: > > > >On 7/27/23 02:43, Xiao Zeng wrote: > >> >> 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. >Sorry to have wasted your time It's okay I am very willing to accept opinions from the gcc community. >-- zicond/xventanacondops has lingered >for quite a while and I had a bit of free time yesterday. I felt it was >most useful to try and move this stuff forward. > > > >> >> 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. >Agreed. > >> >> 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]. >Agreed. > >> >> 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? >I'm going to look at 3/5 today pretty closely. Exposing zicond to >mov<node>cc is something we had implemented inside Ventana and I want to >compare/contrast your work with ours. What a coincidence! > >What I like about yours is it keeps all the logic in riscv.cc rather >than scattering it across riscv.cc and riscv.md. Yes, when I use enough test cases, I cannot find a concise way to optimize all test cases. When I enumerated all possible cases in the mov<mode>cc function of the RISC-V backend, I found a method that satisfied me, which is the method in patch [3/5]. >What I like about the >internal Ventana bits is its ability to support arbitrary comparisons by >utilizing sCC if the original is not an eq/ne comparison. > If it's just for the Zicond instruction set, is it necessary to make judgments outside of eq/ne? After all, it does not support comparison actions other than eq/ne. Of course, it is also possible to use a special technique to use Zicond in non eq/ne comparisons. >Ideally we'll be able to get the best of both. Of course, it is best to unify all situations in one framework. > >Jeff Now that the code on the master has preliminary support for Zicond, I will still submit the optimization patches for Zicond to the community for the convenience of finding the ideal method. Thanks Xiao Zeng
On 7/28/23 00:34, Xiao Zeng wrote: >>> >>> Does that work for you? >> I'm going to look at 3/5 today pretty closely. Exposing zicond to >> mov<node>cc is something we had implemented inside Ventana and I want to >> compare/contrast your work with ours. > > What a coincidence! Zicond is a direct descendant of xventanacondops. The only notable difference is in their encodings. > >> >> What I like about yours is it keeps all the logic in riscv.cc rather >> than scattering it across riscv.cc and riscv.md. > > Yes, when I use enough test cases, I cannot find a concise way to optimize > all test cases. When I enumerated all possible cases in the mov<mode>cc > function of the RISC-V backend, I found a method that satisfied me, which > is the method in patch [3/5]. I got pulled away to another task yesterday, so didn't get as far as I wanted. The biggest inight from yesterday was determining that some of the cases you're handling in riscv_expand_conditional_move were things we were doing inside ifcvt.cc. The difference is likely because the initial work on zicond here was primarily driven by changes to ifcvt. It was only after evaluating that initial implementation that we started to the effort to use zicond at RTL expansion time. I could make a case for either approach, but the more I ponder them the more I'm inclined to go with something like yours. We want to capture the cases implementable as a conditional move as early as possible in the RTL pipeline rather than relying on ifcvt to catch it later. It also avoids polluting ifcvt with transformations that are only likely needed on risc-v. >> > > If it's just for the Zicond instruction set, is it necessary to make judgments > outside of eq/ne? After all, it does not support comparison actions other > than eq/ne. Of course, it is also possible to use a special technique to use > Zicond in non eq/ne comparisons. It's not necessary, but it's definitely helpful to cover the other conditions. In fact, we can even cover a variety of fp conditions by utilizing the sCC type insns. So what I'm looking at for patch #3 is to split out the costing bits into its own patch which can go forward immediately. THen continue evaluating the best way to handle unifying the expander/canonicalization code. Your testcases in patch #3 are particularly helpful to make sure we're not missing cases. Jeff
On Fri, Jul 28, 2023 at 11:03:00 PM Jeff Law <jeffreyalaw@gmail.com> wrote: > > > >On 7/28/23 00:34, Xiao Zeng wrote: > >>>> >>>> Does that work for you? >>> I'm going to look at 3/5 today pretty closely. Exposing zicond to >>> mov<node>cc is something we had implemented inside Ventana and I want to >>> compare/contrast your work with ours. >> >> What a coincidence! >Zicond is a direct descendant of xventanacondops. The only notable >difference is in their encodings. It explains the matter. > >> >>> >>> What I like about yours is it keeps all the logic in riscv.cc rather >>> than scattering it across riscv.cc and riscv.md. >> >> Yes, when I use enough test cases, I cannot find a concise way to optimize >> all test cases. When I enumerated all possible cases in the mov<mode>cc >> function of the RISC-V backend, I found a method that satisfied me, which >> is the method in patch [3/5]. >I got pulled away to another task yesterday, so didn't get as far as I >wanted. The biggest inight from yesterday was determining that some of >the cases you're handling in riscv_expand_conditional_move were things >we were doing inside ifcvt.cc. > >The difference is likely because the initial work on zicond here was >primarily driven by changes to ifcvt. It was only after evaluating that >initial implementation that we started to the effort to use zicond at >RTL expansion time. > >I could make a case for either approach, but the more I ponder them the >more I'm inclined to go with something like yours. >We want to capture >the cases implementable as a conditional move as early as possible in >the RTL pipeline rather than relying on ifcvt to catch it later. It >also avoids polluting ifcvt with transformations that are only likely >needed on risc-v. That's why I did this optimization in riscv.cc riscv_expand_conditional_move. > > >>> >> >> If it's just for the Zicond instruction set, is it necessary to make judgments >> outside of eq/ne? After all, it does not support comparison actions other >> than eq/ne. Of course, it is also possible to use a special technique to use >> Zicond in non eq/ne comparisons. >It's not necessary, but it's definitely helpful to cover the other >conditions. In fact, we can even cover a variety of fp conditions by >utilizing the sCC type insns. It would be great if we could do this. > > >So what I'm looking at for patch #3 is to split out the costing bits >into its own patch which can go forward immediately. As you expected, V2-patch[3/5] has arrived, and its address is: https://gcc.gnu.org/pipermail/gcc-patches/2023-July/625781.html >THen continue >evaluating the best way to handle unifying the expander/canonicalization >code. That's nice. >Your testcases in patch #3 are particularly helpful to make sure >we're not missing cases. Yes, I have always believed that test cases can be redundant, but they cannot be omitted. As we all know, the compiler will always make some magical changes without our knowledge, which may not be what we expect. And test cases can help us stay away from this risk. > >Jeff Thanks Xiao Zeng
On 7/28/23 00:34, Xiao Zeng wrote: > >> >> What I like about yours is it keeps all the logic in riscv.cc rather >> than scattering it across riscv.cc and riscv.md. > > Yes, when I use enough test cases, I cannot find a concise way to optimize > all test cases. When I enumerated all possible cases in the mov<mode>cc > function of the RISC-V backend, I found a method that satisfied me, which > is the method in patch [3/5]. I continue to work with the riscv_expand_conditional_move improvements. Given the deeper problems we have with costing, I'm considering starting to push some of the riscv_expand_conditional_move work you've done without the testcases since those testcases depend on fixing the costing problems. The expansion changes still have value without the costing changes. When we expand a COND_EXPR from gimple, we will attempt to use the conditional move pattern first, without regard for costing. > > If it's just for the Zicond instruction set, is it necessary to make judgments > outside of eq/ne? After all, it does not support comparison actions other > than eq/ne. Of course, it is also possible to use a special technique to use > Zicond in non eq/ne comparisons. It's not necessary, but it's certainly helpful to utilize sCC insns in conjuction with zicond to if-convert other conditional branches. It's conceptually pretty simple. If the incoming code is not EQ/NE or we're not comparing a register against 0, then we can emit an scc insn to get the comparison result into a temporary, then use the standard zicond expansions. Jeff
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 gcc/common/config/riscv/riscv-common.cc | 3 + gcc/config/riscv/riscv-opts.h | 3 + gcc/config/riscv/riscv.cc | 141 +++++ gcc/config/riscv/riscv.md | 3 +- gcc/config/riscv/zicond.md | 84 +++ gcc/ifcvt.cc | 251 ++++++++ gcc/testsuite/gcc.target/riscv/attribute-20.c | 6 + gcc/testsuite/gcc.target/riscv/attribute-21.c | 6 + ...ionalArithmetic_compare_0_return_imm_reg.c | 553 +++++++++++++++++ ...ionalArithmetic_compare_0_return_reg_reg.c | 585 ++++++++++++++++++ ...nalArithmetic_compare_imm_return_imm_reg.c | 297 +++++++++ ...nalArithmetic_compare_imm_return_reg_reg.c | 297 +++++++++ ...nalArithmetic_compare_reg_return_imm_reg.c | 297 +++++++++ ...nalArithmetic_compare_reg_return_reg_reg.c | 329 ++++++++++ .../riscv/zicond-primitiveSemantics.c | 49 ++ .../zicond-primitiveSemantics_compare_imm.c | 57 ++ ...mitiveSemantics_compare_imm_return_0_imm.c | 73 +++ ...tiveSemantics_compare_imm_return_imm_imm.c | 73 +++ ...tiveSemantics_compare_imm_return_imm_reg.c | 65 ++ ...tiveSemantics_compare_imm_return_reg_reg.c | 65 ++ .../zicond-primitiveSemantics_compare_reg.c | 65 ++ ...mitiveSemantics_compare_reg_return_0_imm.c | 73 +++ ...tiveSemantics_compare_reg_return_imm_imm.c | 73 +++ ...tiveSemantics_compare_reg_return_imm_reg.c | 65 ++ ...tiveSemantics_compare_reg_return_reg_reg.c | 77 +++ .../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 ++ 29 files changed, 3857 insertions(+), 1 deletion(-) create mode 100644 gcc/config/riscv/zicond.md create mode 100644 gcc/testsuite/gcc.target/riscv/attribute-20.c create mode 100644 gcc/testsuite/gcc.target/riscv/attribute-21.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-conditionalArithmetic_compare_0_return_imm_reg.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-conditionalArithmetic_compare_0_return_reg_reg.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-conditionalArithmetic_compare_imm_return_imm_reg.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-conditionalArithmetic_compare_imm_return_reg_reg.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-conditionalArithmetic_compare_reg_return_imm_reg.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-conditionalArithmetic_compare_reg_return_reg_reg.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_0_imm.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_imm.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_reg.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_reg_reg.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_0_imm.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_imm.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_reg.c create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_reg_reg.c 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