Thank you @Jeff Law <jeffreya...@gmail.com> for the initial comments ,yes
will update the ChangeLog accordingly and typo fix and
>>My biggest concern is overall structure of
riscv_expand_conditional_move. I kind of get the sense
>>that we need to refactor operand canonicalization into its own routine,
>>then have two subroutines,
Well ,it makes sense to refactor to have two subroutines .
Thank you again for your time ,we will make changes and will send the
updated patch soon .
~U
On Wed, Jun 4, 2025 at 8:22 PM Jeff Law <jeffreya...@gmail.com> wrote:
>
>
> On 5/27/25 5:06 AM, Umesh Kalappa wrote:
> > The P8700 is a high-performance processor from MIPS by extending RISCV
> with
> > the MIPS custom instruction and the following changes are added to
> enable the conditional move support from mips
> >
> > No regressions are found for "runtest --tool gcc
> --target_board='riscv-sim/-mabi=lp64d/-mcmodel=medlow/-mtune=mips-p8700/-O2
> ' riscv.exp"
> >
> > *config/riscv/riscv-cores.def(RISCV_CORE):Updated the march for
> mips-p8700 tune.
> > *config/riscv/riscv-ext-mips.def(DEFINE_RISCV_EXT):
> > New file added for mips conditional mov extension.
> > *config/riscv/riscv-ext.def: Likewise.
> > *config/riscv/t-riscv:Generates riscv-ext.opt
> > *config/riscv/riscv-ext.opt: Generated file.
> > *config/riscv/riscv.cc(riscv_expand_conditional_move):Updated for
> mips cmov.
> > *config/riscv/riscv.md(mov<mode>cc):updated expand for MIPS CCMOV.
> > *config/riscv/mips-insn.md:New file for mips-p8700 ccmov insn.
> > *testsuite/gcc.target/riscv/mipscondmov.c:Test file for mips.ccmov
> insn.
> > *gcc/doc/riscv-ext.texi:Updated for mips cmov.
> > ---
> >
> >
> > diff --git a/gcc/config/riscv/mips-insn.md
> b/gcc/config/riscv/mips-insn.md
> > new file mode 100644
> > index 00000000000..ee106c4221e
> > --- /dev/null
> > +++ b/gcc/config/riscv/mips-insn.md
> > @@ -0,0 +1,37 @@
> > +;; Machine description for MIPS custom instructioins.
> Typo. Should be "instructions".
>
>
>
>
> > diff --git a/gcc/config/riscv/riscv-cores.def
> b/gcc/config/riscv/riscv-cores.def
> > index 118fef23cad..b8bf81e7883 100644
> > --- a/gcc/config/riscv/riscv-cores.def
> > +++ b/gcc/config/riscv/riscv-cores.def
> > @@ -154,7 +154,7 @@ RISCV_CORE("xiangshan-nanhu",
> "rv64imafdc_zba_zbb_zbc_zbs_"
> > "svinval_zicbom_zicboz",
> > "xiangshan-nanhu")
> >
> > -RISCV_CORE("mips-p8700", "rv64imafd_zicsr_zmmul_"
> > +RISCV_CORE("mips-p8700", "rv64imafd_"
> > "zaamo_zalrsc_zba_zbb",
> > "mips-p8700")
> You know your core better than I, so I'm going to assume your
> architecture string is reasonably correct. But the change does not
> match your ChangeLog entry. You're not adjusting tuning at all. This
> adjusts the supported architecture.
>
>
>
> > + /* UPPERCAE_NAME */ XMIPSCMOV,
> Should be "UPPERCASE". I think Kito got it wrong in his original patch
> and the typo keeps getting copied into new files ;(
>
>
>
> > @@ -5352,8 +5352,31 @@ riscv_expand_conditional_move (rtx dest, rtx op,
> rtx cons, rtx alt)
> > rtx_code code = GET_CODE (op);
> > rtx op0 = XEXP (op, 0);
> > rtx op1 = XEXP (op, 1);
> > + rtx target;
> >
> > - if (((TARGET_ZICOND_LIKE
> > + if (TARGET_XMIPSCMOV && mode == word_mode && GET_MODE (op0) ==
> word_mode)
> > + {
> > + if (code == EQ || code == NE)
> > + {
> > + op0 = riscv_zero_if_equal (op0, op1);
> > + op1 = const0_rtx;
> > + }
> Your formatting looks goofy here. The open-curley appears to be
> indented one space inside the IF. Occasionally this is due to tabs, so
> double check it may be a false positive.
>
>
> > + else
> > + {
> > + target = gen_reg_rtx (GET_MODE (op0));
> > + riscv_emit_int_order_test(code, 0, target, op0, op1);
> Always a space between the function name and the open paren for the
> argument list.
>
> Given your semantics are full conditional move rather than zicond I
> wonder if we should restructure this code a little. Other than things
> like canonicalization of the condition I doubt we're going to be able to
> share much between the zicond forms and full conditional move forms.
>
>
>
> > diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> > index 92fe7c7741a..0c25a723d9c 100644
> > --- a/gcc/config/riscv/riscv.md
> > +++ b/gcc/config/riscv/riscv.md
> > @@ -3290,8 +3290,18 @@
> > (match_operand:GPR 2 "movcc_operand")
> > (match_operand:GPR 3 "movcc_operand")))]
> > "TARGET_SFB_ALU || TARGET_XTHEADCONDMOV || TARGET_ZICOND_LIKE
> > - || TARGET_MOVCC"
> > + || TARGET_MOVCC || TARGET_XMIPSCMOV"
> > {
> > + if (TARGET_XMIPSCMOV) {
> > + /* operands[2] must be register or 0. */
> > + if (!reg_or_0_operand (operands[2], GET_MODE (operands[2])))
> > + operands[2] = force_reg (<GPR:MODE>mode, operands[2]);
> > +
> > + /* operands[3] must be register or 0. */
> > + if (!reg_or_0_operand (operands[3], GET_MODE (operands[3])))
> > + operands[3] = force_reg (<GPR:MODE>mode, operands[3]);
> > + }
> Can this be pushed into riscv_expand_conditional_move? Essentially with
> your patch we have two places where operands are adjusted. We should
> avoid that if we easily can.
>
>
> Overall it looks pretty sensible. My biggest concern is overall
> structure of riscv_expand_conditional_move. I kind of get the sense
> that we need to refactor operand canonicalization into its own routine,
> then have two subroutines, one for the zicond like variants and another
> for the full conditional move rather than trying to handle them both
> inside riscv_expand_conditional_move given I don't expect them to share
> much code.
>
> Thoughts?
>
> jeff
>
