RE: [PATCH v8] RISC-V: Support CALL for RVV floating-point dynamic rounding
Committed, thanks a lof, Robin and Kito, very appreciative for the explanation and comments from the expert's perspective. Pan -Original Message- From: Kito Cheng Sent: Tuesday, August 1, 2023 3:51 PM To: Li, Pan2 Cc: Robin Dapp ; gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@sifive.com; Wang, Yanzhang Subject: Re: [PATCH v8] RISC-V: Support CALL for RVV floating-point dynamic rounding Hi Pan: Thanks for your effort on this, this is LGTM and OK for trunk. Hi Robin: Thanks for your review on this stuff, this set of intrinsic functions is complicated and might be controversial since the whole floating point rounding mode is…complicated, and people might have different tastes on that. So what we (RVV intrinsic TG) trying to do is adding another set of intrinsic *and* keep existing floating point intrinsic, so people could still using fesetround style to play around the floating point stuffs, but I am not intend to convince you that is necessary and it's 100% right design - I admit it's kind of experimental design as the LLVM's constrained floating-point intrinsics. Anyway, let's move forward, and see how useful it is for the RISC-V ecosystem :) On Fri, Jul 28, 2023 at 8:35 PM Li, Pan2 via Gcc-patches wrote: > > Great! Thanks Robin for so many useful comments, as well as the > thought-provoking discussion with different insights. > I believe such kind of interactively discussion will empower all of us, and > leading us to do the right things. > > Back to this PATCH, I try to only do one thing at a time and I totally agree > that there are something we need to try. > Thanks again and let's wait for kito's comments. > > Pan > > -Original Message- > From: Robin Dapp > Sent: Friday, July 28, 2023 6:05 PM > To: Li, Pan2 ; gcc-patches@gcc.gnu.org > Cc: rdapp@gmail.com; juzhe.zh...@rivai.ai; kito.ch...@sifive.com; Wang, > Yanzhang > Subject: Re: [PATCH v8] RISC-V: Support CALL for RVV floating-point dynamic > rounding > > Hi Pan, > > thanks for your patience and your work. Apart from my general doubt > whether mode-changing intrinsics are a good idea, I don't have other > remarks that need fixing. What I mentioned before: > > - Handling of asms wouldn't be a huge change. It can be done > in a follow-up patch of course but should be done eventually. > > - The code is still rather difficult to follow because we diverge > from the usual mode-switching semantics e.g. in that we emit insns > in mode_needed as well as in mode_set. I would have preferred > to stay close to the regular usage, document where and why we need > to do something different and suggest future middle-end improvements > to solve this more elegantly. > > - I hope non-local control flow like setjmp/longjmp, sibcall > optimization and maybe others work fine. I didn't see a reason > why not but I haven't checked very closely either. > > - We can probably get away with not annotating every call with > an FRM clobber because there isn't any pass that would make use > of that anyway? > > > As to my general qualm, independent of this patch, quickly > summarized again one last time (the problem was latent before this > specific patch anyway): > > I would prefer not to have mode-changing intrinsics at all but > have users call fesetround explicitly. That way the exact point > where the rounding mode is changed would be obvious and not > subject to optimization as well as caching/backing up. > If at all necessary I would have preferred the LLVM way of > backing up, setting new mode, performing the instruction > and restoring directly after. > If the initial intent of mode-changing intrinsics was to give > users more control, I don't believe we achieve this by the "lazy" > restore mechanism which is rather an obfuscation. > > Pardon my frankness but the whole mode-changing thing feels to me > like just getting a feature out of the door to solve "something" > /appease users than a well thought-out feature. It doesn't even > seem clear if this optimization is worthwhile when changing the > rounding mode is prohibitively slow anyway. > > That said, if the current status is what the majority of > contributors can live with, I'm not going to stand in the way, > but I'd ask Kito or somebody else to give the final OK. > > Regards > Robin
Re: [PATCH v8] RISC-V: Support CALL for RVV floating-point dynamic rounding
Hi Pan: Thanks for your effort on this, this is LGTM and OK for trunk. Hi Robin: Thanks for your review on this stuff, this set of intrinsic functions is complicated and might be controversial since the whole floating point rounding mode is…complicated, and people might have different tastes on that. So what we (RVV intrinsic TG) trying to do is adding another set of intrinsic *and* keep existing floating point intrinsic, so people could still using fesetround style to play around the floating point stuffs, but I am not intend to convince you that is necessary and it's 100% right design - I admit it's kind of experimental design as the LLVM's constrained floating-point intrinsics. Anyway, let's move forward, and see how useful it is for the RISC-V ecosystem :) On Fri, Jul 28, 2023 at 8:35 PM Li, Pan2 via Gcc-patches wrote: > > Great! Thanks Robin for so many useful comments, as well as the > thought-provoking discussion with different insights. > I believe such kind of interactively discussion will empower all of us, and > leading us to do the right things. > > Back to this PATCH, I try to only do one thing at a time and I totally agree > that there are something we need to try. > Thanks again and let's wait for kito's comments. > > Pan > > -Original Message- > From: Robin Dapp > Sent: Friday, July 28, 2023 6:05 PM > To: Li, Pan2 ; gcc-patches@gcc.gnu.org > Cc: rdapp@gmail.com; juzhe.zh...@rivai.ai; kito.ch...@sifive.com; Wang, > Yanzhang > Subject: Re: [PATCH v8] RISC-V: Support CALL for RVV floating-point dynamic > rounding > > Hi Pan, > > thanks for your patience and your work. Apart from my general doubt > whether mode-changing intrinsics are a good idea, I don't have other > remarks that need fixing. What I mentioned before: > > - Handling of asms wouldn't be a huge change. It can be done > in a follow-up patch of course but should be done eventually. > > - The code is still rather difficult to follow because we diverge > from the usual mode-switching semantics e.g. in that we emit insns > in mode_needed as well as in mode_set. I would have preferred > to stay close to the regular usage, document where and why we need > to do something different and suggest future middle-end improvements > to solve this more elegantly. > > - I hope non-local control flow like setjmp/longjmp, sibcall > optimization and maybe others work fine. I didn't see a reason > why not but I haven't checked very closely either. > > - We can probably get away with not annotating every call with > an FRM clobber because there isn't any pass that would make use > of that anyway? > > > As to my general qualm, independent of this patch, quickly > summarized again one last time (the problem was latent before this > specific patch anyway): > > I would prefer not to have mode-changing intrinsics at all but > have users call fesetround explicitly. That way the exact point > where the rounding mode is changed would be obvious and not > subject to optimization as well as caching/backing up. > If at all necessary I would have preferred the LLVM way of > backing up, setting new mode, performing the instruction > and restoring directly after. > If the initial intent of mode-changing intrinsics was to give > users more control, I don't believe we achieve this by the "lazy" > restore mechanism which is rather an obfuscation. > > Pardon my frankness but the whole mode-changing thing feels to me > like just getting a feature out of the door to solve "something" > /appease users than a well thought-out feature. It doesn't even > seem clear if this optimization is worthwhile when changing the > rounding mode is prohibitively slow anyway. > > That said, if the current status is what the majority of > contributors can live with, I'm not going to stand in the way, > but I'd ask Kito or somebody else to give the final OK. > > Regards > Robin
RE: [PATCH v8] RISC-V: Support CALL for RVV floating-point dynamic rounding
Great! Thanks Robin for so many useful comments, as well as the thought-provoking discussion with different insights. I believe such kind of interactively discussion will empower all of us, and leading us to do the right things. Back to this PATCH, I try to only do one thing at a time and I totally agree that there are something we need to try. Thanks again and let's wait for kito's comments. Pan -Original Message- From: Robin Dapp Sent: Friday, July 28, 2023 6:05 PM To: Li, Pan2 ; gcc-patches@gcc.gnu.org Cc: rdapp@gmail.com; juzhe.zh...@rivai.ai; kito.ch...@sifive.com; Wang, Yanzhang Subject: Re: [PATCH v8] RISC-V: Support CALL for RVV floating-point dynamic rounding Hi Pan, thanks for your patience and your work. Apart from my general doubt whether mode-changing intrinsics are a good idea, I don't have other remarks that need fixing. What I mentioned before: - Handling of asms wouldn't be a huge change. It can be done in a follow-up patch of course but should be done eventually. - The code is still rather difficult to follow because we diverge from the usual mode-switching semantics e.g. in that we emit insns in mode_needed as well as in mode_set. I would have preferred to stay close to the regular usage, document where and why we need to do something different and suggest future middle-end improvements to solve this more elegantly. - I hope non-local control flow like setjmp/longjmp, sibcall optimization and maybe others work fine. I didn't see a reason why not but I haven't checked very closely either. - We can probably get away with not annotating every call with an FRM clobber because there isn't any pass that would make use of that anyway? As to my general qualm, independent of this patch, quickly summarized again one last time (the problem was latent before this specific patch anyway): I would prefer not to have mode-changing intrinsics at all but have users call fesetround explicitly. That way the exact point where the rounding mode is changed would be obvious and not subject to optimization as well as caching/backing up. If at all necessary I would have preferred the LLVM way of backing up, setting new mode, performing the instruction and restoring directly after. If the initial intent of mode-changing intrinsics was to give users more control, I don't believe we achieve this by the "lazy" restore mechanism which is rather an obfuscation. Pardon my frankness but the whole mode-changing thing feels to me like just getting a feature out of the door to solve "something" /appease users than a well thought-out feature. It doesn't even seem clear if this optimization is worthwhile when changing the rounding mode is prohibitively slow anyway. That said, if the current status is what the majority of contributors can live with, I'm not going to stand in the way, but I'd ask Kito or somebody else to give the final OK. Regards Robin
Re: [PATCH v8] RISC-V: Support CALL for RVV floating-point dynamic rounding
Hi Pan, thanks for your patience and your work. Apart from my general doubt whether mode-changing intrinsics are a good idea, I don't have other remarks that need fixing. What I mentioned before: - Handling of asms wouldn't be a huge change. It can be done in a follow-up patch of course but should be done eventually. - The code is still rather difficult to follow because we diverge from the usual mode-switching semantics e.g. in that we emit insns in mode_needed as well as in mode_set. I would have preferred to stay close to the regular usage, document where and why we need to do something different and suggest future middle-end improvements to solve this more elegantly. - I hope non-local control flow like setjmp/longjmp, sibcall optimization and maybe others work fine. I didn't see a reason why not but I haven't checked very closely either. - We can probably get away with not annotating every call with an FRM clobber because there isn't any pass that would make use of that anyway? As to my general qualm, independent of this patch, quickly summarized again one last time (the problem was latent before this specific patch anyway): I would prefer not to have mode-changing intrinsics at all but have users call fesetround explicitly. That way the exact point where the rounding mode is changed would be obvious and not subject to optimization as well as caching/backing up. If at all necessary I would have preferred the LLVM way of backing up, setting new mode, performing the instruction and restoring directly after. If the initial intent of mode-changing intrinsics was to give users more control, I don't believe we achieve this by the "lazy" restore mechanism which is rather an obfuscation. Pardon my frankness but the whole mode-changing thing feels to me like just getting a feature out of the door to solve "something" /appease users than a well thought-out feature. It doesn't even seem clear if this optimization is worthwhile when changing the rounding mode is prohibitively slow anyway. That said, if the current status is what the majority of contributors can live with, I'm not going to stand in the way, but I'd ask Kito or somebody else to give the final OK. Regards Robin
[PATCH v8] RISC-V: Support CALL for RVV floating-point dynamic rounding
From: Pan Li Update in PATCH v8: 1. Emit non-abnormal backup insn to edge. 2. Fix _after return when call. 3. Refine some run tests. 4. Cleanup code. Original commit logs: In basic dynamic rounding mode, we simply ignore call instructions and we would like to take care of call in this PATCH. During the call, the frm may be updated or keep as is. Thus, we must make sure at least 2 things. 1. The static frm before call should not pollute the frm value in call. 2. The updated frm value in call should be sticky after call completed. We will perfrom some steps to make above happen. 1. Mark call instruction with new mode DYN_CALL. 2. Mark the instruction after CALL from NONE to DYN. 3. When emit for a DYN_CALL, we will restore the frm value. 4. When emit from a DYN_CALL, we will backup the frm value. Let's take a flow for this. +-+ | Entry (DYN) | <- frrm a5 +-+ / \ +---+ +---+ | VFADD | | VFADD RTZ | <- fsrmi 1(RTZ) +---+ +---+ || +---+ +---+ | CALL | | CALL | <- fsrm a5 +---+ +---+ | | +---+ +---+ | SHIFT | <- frrm a5 | VFADD | <- frrm a5 +---+ +---+ | / +---+ / | VFADD RUP | <- fsrm1 3(RUP) +---+ / \ / +-+ | Exit (DYN_EXIT) | <- fsrm a5 +-+ When call is the last insn of one bb, we take care of it when needed for each insn by inserting one frm backup (frrm) insn to the end of the current bb. Signed-off-by: Pan Li Co-Authored-By: Juzhe-Zhong gcc/ChangeLog: * config/riscv/riscv.cc (DYNAMIC_FRM_RTL): New macro. (STATIC_FRM_P): Ditto. (struct mode_switching_info): New struct for mode switching. (struct machine_function): Add new field mode switching. (riscv_emit_frm_mode_set): Add DYN_CALL emit. (riscv_frm_adjust_mode_after_call): New function for call mode. (riscv_frm_emit_after_call_in_bb_end): New function for emit insn when call as the end of bb. (riscv_frm_mode_needed): New function for frm mode needed. (frm_unknown_dynamic_p): Remove call check. (riscv_mode_needed): Extrac function for frm. (riscv_frm_mode_after): Add DYN_CALL after. (riscv_mode_entry): Remove backup rtl initialization. * config/riscv/vector.md (frm_mode): Add dyn_call. (fsrmsi_restore_exit): Rename to _volatile. (fsrmsi_restore_volatile): Likewise. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/base/float-point-frm-insert-7.c: Adjust test cases. * gcc.target/riscv/rvv/base/float-point-frm-run-1.c: Ditto. * gcc.target/riscv/rvv/base/float-point-frm-run-2.c: Ditto. * gcc.target/riscv/rvv/base/float-point-frm-run-3.c: Ditto. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-33.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-34.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-35.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-36.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-37.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-38.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-39.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-40.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-41.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-42.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-43.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-44.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-45.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-46.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-47.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-48.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-49.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-50.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-51.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-52.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-53.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-54.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-55.c: New test. * gcc.target/riscv/rvv/base/float-point-dynamic-frm-56.c: New test. *
