Re: [PATCH v2] RISC-V: Refactor riscv mode after for VXRM and FRM

[Kito Cheng <kito.cheng@gmail.com>]

oh, I know why you failed on that, you need to put it within the
function, not global static, function static variable will construct
when first invoked rather than construct at program start.

Could you try to apply my diff in the last mail and try again?

On Thu, Jul 13, 2023 at 3:29 PM Li, Pan2 via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Thanks Kito for review. Sorry didn't involve the code result in self test error in PATCH v3, but it can be reproduced with below diff based on PATCH v3. Let me know if I didn't get the point of your comments.
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 6ed735d6983..76689eaf8d5 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -233,6 +233,9 @@ static int epilogue_cfa_sp_offset;
>  /* Which tuning parameters to use.  */
>  static const struct riscv_tune_param *tune_param;
>
> +static const_rtx vxrm_rtx = gen_rtx_REG (SImode, VXRM_REGNUM);
> +static const_rtx frm_rtx = gen_rtx_REG (SImode, FRM_REGNUM);
> +
>  /* Which automaton to use for tuning.  */
>  enum riscv_microarchitecture_type riscv_microarchitecture;
>
> @@ -7717,7 +7720,7 @@ static bool
>  vxrm_unknown_p (rtx_insn *insn)
>  {
>    /* Return true if there is a definition of VXRM.  */
> -  if (reg_set_p (gen_rtx_REG (SImode, VXRM_REGNUM), insn))
> +  if (reg_set_p (vxrm_rtx, insn))
>      return true;
>
>    /* A CALL function may contain an instruction that modifies the VXRM,
> @@ -7739,7 +7742,7 @@ static bool
>  frm_unknown_dynamic_p (rtx_insn *insn)
>  {
>    /* Return true if there is a definition of FRM.  */
> -  if (reg_set_p (gen_rtx_REG (SImode, FRM_REGNUM), insn))
> +  if (reg_set_p (frm_rtx, insn))
>      return true;
>
>    /* A CALL function may contain an instruction that modifies the FRM,
> @@ -7761,7 +7764,7 @@ riscv_vxrm_mode_after (rtx_insn *insn, int mode)
>    if (recog_memoized (insn) < 0)
>      return mode;
>
> -  if (reg_mentioned_p (gen_rtx_REG (SImode, VXRM_REGNUM), PATTERN (insn)))
> +  if (reg_mentioned_p (vxrm_rtx, PATTERN (insn)))
>      return get_attr_vxrm_mode (insn);
>    else
>      return mode;
> @@ -7778,7 +7781,7 @@ riscv_frm_mode_after (rtx_insn *insn, int mode)
>    if (recog_memoized (insn) < 0)
>      return mode;
>
> -  if (reg_mentioned_p (gen_rtx_REG (SImode, FRM_REGNUM), PATTERN (insn)))
> +  if (reg_mentioned_p (frm_rtx, PATTERN (insn)))
>      return get_attr_frm_mode (insn);
>    else
>      return mode;
>
> Pan
>
> -----Original Message-----
> From: Kito Cheng <kito.cheng@gmail.com>
> Sent: Thursday, July 13, 2023 2:19 PM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: Jeff Law <jeffreyalaw@gmail.com>; gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>
> Subject: Re: [PATCH v2] RISC-V: Refactor riscv mode after for VXRM and FRM
>
> Hmmm? I didn't get that error on selftest?
>
> my diff with your v2:
>
> $ git diff
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 12655f7fdc65..466e1aed91c7 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -8058,8 +8058,9 @@ asm_insn_p (rtx_insn *insn)
> static bool
> vxrm_unknown_p (rtx_insn *insn)
> {
> +  static const_rtx vxrm_reg = gen_rtx_REG (SImode, VXRM_REGNUM);
>   /* Return true if there is a definition of VXRM.  */
> -  if (reg_set_p (gen_rtx_REG (SImode, VXRM_REGNUM), insn))
> +  if (reg_set_p (vxrm_reg, insn))
>     return true;
>
>   /* A CALL function may contain an instruction that modifies the VXRM,
> @@ -8080,8 +8081,9 @@ vxrm_unknown_p (rtx_insn *insn)
> static bool
> frm_unknown_dynamic_p (rtx_insn *insn)
> {
> +  static const_rtx frm_reg = gen_rtx_REG (SImode, FRM_REGNUM);
>   /* Return true if there is a definition of FRM.  */
> -  if (reg_set_p (gen_rtx_REG (SImode, FRM_REGNUM), insn))
> +  if (reg_set_p (frm_reg, insn))
>     return true;
>
>   /* A CALL function may contain an instruction that modifies the FRM,
>
>
> On Thu, Jul 13, 2023 at 1:07 PM Li, Pan2 via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > Thanks Jeff and Kito for comments, update the V3 version as below.
> >
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-July/624347.html
> >
> > > Extract vxrm reg to a local static variable to prevent construct that again and again.
> >
> > The "static const_rtx vxrm_rtx = gen_rtx_REG (SImode, VXRM_REGMU)" results in some error when selftest like below, thus patch v3 doesn't include this change.
> >
> > /home/pli/repos/gcc/111/riscv-gnu-toolchain/build-gcc-newlib-stage1/./gcc/xgcc -B/home/pli/repos/gcc/111/riscv-gnu-toolchain/build-gcc-newlib-stage1/./gcc/  -xc -nostdinc /dev/null -S -o /dev/null -fself-test=../.././gcc/gcc/testsuite/selftests
> > virtual memory exhausted: Invalid argument
> > make[2]: *** [../.././gcc/gcc/c/Make-lang.in:153: s-selftest-c] Error 1
> >
> > Pan
> >
> > -----Original Message-----
> > From: Jeff Law <jeffreyalaw@gmail.com>
> > Sent: Wednesday, July 12, 2023 11:31 PM
> > To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
> > Cc: juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com
> > Subject: Re: [PATCH v2] RISC-V: Refactor riscv mode after for VXRM and FRM
> >
> >
> >
> > On 7/11/23 23:50, pan2.li@intel.com wrote:
> > > From: Pan Li <pan2.li@intel.com>
> > >
> > > When investigate the FRM dynmaic rounding mode, we find the global
> > > unknown status is quite different between the fixed-point and
> > > floating-point. Thus, we separate the unknown function with extracting
> > > some inner common functions.
> > >
> > > We will also prepare more test cases in another PATCH.
> > >
> > > Signed-off-by: Pan Li <pan2.li@intel.com>
> > >
> > > gcc/ChangeLog:
> > >
> > >       * config/riscv/riscv.cc (regnum_definition_p): New function.
> > >       (insn_asm_p): Ditto.
> > >       (riscv_vxrm_mode_after): New function for fixed-point.
> > >       (global_vxrm_state_unknown_p): Ditto.
> > >       (riscv_frm_mode_after): New function for floating-point.
> > >       (global_frm_state_unknown_p): Ditto.
> > >       (riscv_mode_after): Leverage new functions.
> > >       (riscv_entity_mode_after): Removed.
> > > ---
> > >   gcc/config/riscv/riscv.cc | 96 +++++++++++++++++++++++++++++++++------
> > >   1 file changed, 82 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > > index 38d8eb2fcf5..553fbb4435a 100644
> > > --- a/gcc/config/riscv/riscv.cc
> > > +++ b/gcc/config/riscv/riscv.cc
> > > @@ -7742,19 +7742,91 @@ global_state_unknown_p (rtx_insn *insn, unsigned int regno)
> > >     return false;
> > >   }
> > >
> > > +static bool
> > > +regnum_definition_p (rtx_insn *insn, unsigned int regno)
> > Needs a function comment.  This is true for each new function added.  In
> > this specific case somethign like this might be appropriate
> >
> > /* Return TRUE if REGNO is set in INSN, FALSE otherwise.  */
> >
> > Which begs the question, is there some reason why we're not using the
> > existing reg_set_p or simple_regno_set from rtlanal.cc?
> >
> >
> >
> > Jeff