Sure and committed, thanks Kito.
Pan
-----Original Message-----
From: Kito Cheng <kito.ch...@gmail.com>
Sent: Thursday, July 13, 2023 5:19 PM
To: Li, Pan2 <pan2...@intel.com>
Cc: Jeff Law <jeffreya...@gmail.com>; gcc-patches@gcc.gnu.org;
juzhe.zh...@rivai.ai; rdapp....@gmail.com; Wang, Yanzhang
<yanzhang.w...@intel.com>
Subject: Re: [PATCH v2] RISC-V: Refactor riscv mode after for VXRM and FRM
Hmmm, anyway, I guess it's not worth spending any more of your time,
LGTM for v3 :)
On Thu, Jul 13, 2023 at 5:10 PM Li, Pan2 via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> It can pass the selftest with below diff based on v3, but got ICE when build
> newlib.
>
> /home/pli/repos/gcc/222/riscv-gnu-toolchain/newlib/newlib/libc/time/../time/strftime.c:1426:1:
> internal compiler error: in reg_overlap_mentioned_p, at rtlanal.cc:1928
> 1426 | }
> | ^
> 0x87241f reg_overlap_mentioned_p(rtx_def const*, rtx_def const*)
> ../.././gcc/gcc/rtlanal.cc:1928
> 0x1005eab set_of_1
> ../.././gcc/gcc/rtlanal.cc:1440
> 0x10015c2 set_of(rtx_def const*, rtx_def const*)
> ../.././gcc/gcc/rtlanal.cc:1452
> 0x10015c2 reg_set_p(rtx_def const*, rtx_def const*)
> ../.././gcc/gcc/rtlanal.cc:1295
> 0x13f66c0 vxrm_unknown_p
> ../.././gcc/gcc/config/riscv/riscv.cc:7720
> 0x13f66c0 riscv_vxrm_mode_after
> ../.././gcc/gcc/config/riscv/riscv.cc:7760
> 0x13f66c0 riscv_mode_after
> ../.././gcc/gcc/config/riscv/riscv.cc:7799
> 0x1defe69 optimize_mode_switching
> ../.././gcc/gcc/mode-switching.cc:632
> 0x1defe69 execute
> ../.././gcc/gcc/mode-switching.cc:909
>
>
> Diff based on PATCH v3.
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 6ed735d6983..d66ba0030eb 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -7714,10 +7714,10 @@ asm_insn_p (rtx_insn *insn)
> /* Return TRUE that an insn is unknown for VXRM. */
>
> static bool
> -vxrm_unknown_p (rtx_insn *insn)
> +vxrm_unknown_p (rtx_insn *insn, const_rtx vxrm_reg)
> {
> /* 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,
> @@ -7736,10 +7736,10 @@ vxrm_unknown_p (rtx_insn *insn)
> /* Return TRUE that an insn is unknown dynamic for FRM. */
>
> static bool
> -frm_unknown_dynamic_p (rtx_insn *insn)
> +frm_unknown_dynamic_p (rtx_insn *insn, const_rtx frm_reg)
> {
> /* 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,
> @@ -7755,13 +7755,15 @@ frm_unknown_dynamic_p (rtx_insn *insn)
> static int
> riscv_vxrm_mode_after (rtx_insn *insn, int mode)
> {
> - if (vxrm_unknown_p (insn))
> + static const_rtx vxrm_reg = gen_rtx_REG (SImode, VXRM_REGNUM);
> +
> + if (vxrm_unknown_p (insn, vxrm_reg))
> return VXRM_MODE_NONE;
>
> if (recog_memoized (insn) < 0)
> return mode;
>
> - if (reg_mentioned_p (gen_rtx_REG (SImode, VXRM_REGNUM), PATTERN (insn)))
> + if (reg_mentioned_p (vxrm_reg, PATTERN (insn)))
> return get_attr_vxrm_mode (insn);
> else
> return mode;
> @@ -7772,13 +7774,15 @@ riscv_vxrm_mode_after (rtx_insn *insn, int mode)
> static int
> riscv_frm_mode_after (rtx_insn *insn, int mode)
> {
> - if (frm_unknown_dynamic_p (insn))
> + static const_rtx frm_reg = gen_rtx_REG (SImode, FRM_REGNUM);
> +
> + if (frm_unknown_dynamic_p (insn, frm_reg))
> return FRM_MODE_DYN;
>
> if (recog_memoized (insn) < 0)
> return mode;
>
> - if (reg_mentioned_p (gen_rtx_REG (SImode, FRM_REGNUM), PATTERN (insn)))
> + if (reg_mentioned_p (frm_reg, PATTERN (insn)))
> return get_attr_frm_mode (insn);
> else
> return mode;
>
> Pan
>
> -----Original Message-----
> From: Li, Pan2
> Sent: Thursday, July 13, 2023 4:42 PM
> To: Kito Cheng <kito.ch...@gmail.com>
> Cc: Jeff Law <jeffreya...@gmail.com>; gcc-patches@gcc.gnu.org;
> juzhe.zh...@rivai.ai; rdapp....@gmail.com; Wang, Yanzhang
> <yanzhang.w...@intel.com>
> Subject: RE: [PATCH v2] RISC-V: Refactor riscv mode after for VXRM and FRM
>
> Sure thing, get you point now, will have a try and send v4 if everything goes
> well.
>
> Pan
>
> -----Original Message-----
> From: Kito Cheng <kito.ch...@gmail.com>
> Sent: Thursday, July 13, 2023 3:35 PM
> To: Li, Pan2 <pan2...@intel.com>
> Cc: Jeff Law <jeffreya...@gmail.com>; gcc-patches@gcc.gnu.org;
> juzhe.zh...@rivai.ai; rdapp....@gmail.com; Wang, Yanzhang
> <yanzhang.w...@intel.com>
> Subject: Re: [PATCH v2] RISC-V: Refactor riscv mode after for VXRM and FRM
>
> 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.ch...@gmail.com>
> > Sent: Thursday, July 13, 2023 2:19 PM
> > To: Li, Pan2 <pan2...@intel.com>
> > Cc: Jeff Law <jeffreya...@gmail.com>; gcc-patches@gcc.gnu.org;
> > juzhe.zh...@rivai.ai; rdapp....@gmail.com; Wang, Yanzhang
> > <yanzhang.w...@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 <jeffreya...@gmail.com>
> > > Sent: Wednesday, July 12, 2023 11:31 PM
> > > To: Li, Pan2 <pan2...@intel.com>; gcc-patches@gcc.gnu.org
> > > Cc: juzhe.zh...@rivai.ai; rdapp....@gmail.com; Wang, Yanzhang
> > > <yanzhang.w...@intel.com>; kito.ch...@gmail.com
> > > Subject: Re: [PATCH v2] RISC-V: Refactor riscv mode after for VXRM and FRM
> > >
> > >
> > >
> > > On 7/11/23 23:50, pan2...@intel.com wrote:
> > > > From: Pan Li <pan2...@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...@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
