Thank you @Jeff Law <jeffreya...@gmail.com> for the comments and
>>I suspect there's something goofy in the indentation in the block above.
like clang-format ,does gcc have a code formatter we can use ?
Thank you again
~U
On Mon, Jun 30, 2025 at 12:33 AM Jeff Law <jeffreya...@gmail.com> wrote:
>
>
> On 6/13/25 9:01 AM, Umesh Kalappa wrote:
> > Addressed the most of comments and tried to refactor the
> riscv_expand_conditional_move() to some extent.
> >
> > No regressions are found for "runtest --tool gcc
> --target_board='riscv-sim/-mabi=lp64d/-mtune=mips-p8700/-O2 ' riscv.exp"
> >
> > *config/riscv/riscv-cores.def(RISCV_CORE):Updated the supported
> march.
> > *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
> > and outlined some code that handle arch cond move.
> > *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.
> > ---
> > gcc/config/riscv/mips-insn.md | 37 ++++++
> > gcc/config/riscv/riscv-cores.def | 3 +-
> > gcc/config/riscv/riscv-ext-mips.def | 35 +++++
> > gcc/config/riscv/riscv-ext.def | 1 +
> > gcc/config/riscv/riscv-ext.opt | 4 +
> > gcc/config/riscv/riscv.cc | 131 ++++++++++++-------
> > gcc/config/riscv/riscv.md | 3 +-
> > gcc/config/riscv/t-riscv | 3 +-
> > gcc/doc/riscv-ext.texi | 4 +
> > gcc/testsuite/gcc.target/riscv/mipscondmov.c | 30 +++++
> > 10 files changed, 202 insertions(+), 49 deletions(-)
> > create mode 100644 gcc/config/riscv/mips-insn.md
> > create mode 100644 gcc/config/riscv/riscv-ext-mips.def
> > create mode 100644 gcc/testsuite/gcc.target/riscv/mipscondmov.c
> >
>
> > +
> > +(define_insn "*mov<GPR:mode><X:mode>cc_bitmanip"
> > + [(set (match_operand:GPR 0 "register_operand" "=r")
> > + (if_then_else:GPR
> > + (match_operator 5 "equality_operator"
> > + [(match_operand:X 1 "register_operand" "r")
> > + (match_operand:X 2 "const_0_operand" "J")])
> > + (match_operand:GPR 3 "reg_or_0_operand" "rJ")
> > + (match_operand:GPR 4 "reg_or_0_operand" "rJ")))]
> > + "TARGET_XMIPSCMOV"
> Not something you need to fix to move forward, but more something to
> keep in mind.
>
> For the kind of cases in this pattern it may be better to use a code
> iterator rather than a match_operator. You can use iterators much like
> you could other RTL codes. We already have "any_eq" defined for the
> RISC-V port.
>
> And you can trivially get the actual code via a code_attr construct if
> you need to.
>
>
>
>
>
> > +
> > +/* Emit target specific conditional move like TARGET_XMIPSCMOV etc*/
> > +bool
> > +riscv_target_conditional_move (rtx dest, rtx op0, rtx op1, rtx_code
> code,
> > + rtx cons, rtx alt)
> > +{
> > + machine_mode dst_mode = GET_MODE (dest);
> > + rtx target;
> > +
> > + /* force the operands to the register*/
> > + cons = force_reg (dst_mode, cons);
> > + alt = force_reg (dst_mode, alt);
> > +
> > + if (TARGET_XMIPSCMOV)
> > + {
> > + if (code == EQ || code == NE)
> > + {
> > + op0 = riscv_zero_if_equal (op0, op1);
> > + op1 = const0_rtx;
> > + }
> > + else
> > + {
> > + target = gen_reg_rtx (GET_MODE (op0));
> > + riscv_emit_int_order_test(code, 0, target, op0, op1);
> > + op0 = target;
> > + op1 = const0_rtx;
> > + code = NE;
> > + }
> I suspect there's something goofy in the indentation in the block above.
> Each indention level is 2 spaces and when you get 8 spaces of
> indention you should use a tab.
>
> Space before the open paren in the call to riscv_emit_int_order_test.
>
>
> > @@ -5454,34 +5520,22 @@ riscv_expand_conditional_move (rtx dest, rtx op,
> rtx cons, rtx alt)
> > @@ -5500,9 +5554,9 @@ riscv_expand_conditional_move (rtx dest, rtx op,
> rtx cons, rtx alt)
> > if (code == LE || code == LEU || code == GE || code == GEU)
> > invert_ptr = &invert;
> >
> > - /* Emit an SCC-like instruction into a temporary so that we can
> > - use an EQ/NE comparison. We can support both FP and integer
> > - conditional moves. */
> > + /* Emit an SCC-like instruction into a temporary so that we can
> > + use an EQ/NE comparison. We can support both FP and integer
> > + conditional moves. */
> > if (INTEGRAL_MODE_P (mode0))
> > riscv_expand_int_scc (tmp, code, op0, op1, invert_ptr);
> > else if (FLOAT_MODE_P (mode0)
> Not sure why you changed the indentation here. I just checked the
> current version of this code on the trunk and the indentation seems
> correct there.
>
> We don't mind indentation/whitespace fixes, but be sure you're actually
> fixing something rather than making gratutious changes.
>
>
>
>
> > @@ -5558,15 +5599,15 @@ riscv_expand_conditional_move (rtx dest, rtx op,
> rtx cons, rtx alt)
> > else if (cons == CONST0_RTX (dst_mode)
> > && ((REG_P (alt) || SUBREG_P (alt))
> > || (CONST_INT_P (alt) && alt != CONST0_RTX (dst_mode))))
> > - {
> > - riscv_emit_int_compare (&code, &op0, &op1, true);
> > - rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
> > - alt = force_reg (dst_mode, alt);
> > - emit_insn (gen_rtx_SET (dest,
> > - gen_rtx_IF_THEN_ELSE (dst_mode, cond,
> > - cons, alt)));
> > - return true;
> > - }
> > + {
> > + riscv_emit_int_compare (&code, &op0, &op1, true);
> > + rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
> > + alt = force_reg (dst_mode, alt);
> > + emit_insn (gen_rtx_SET (dest,
> > + gen_rtx_IF_THEN_ELSE (dst_mode, cond,
> > + cons, alt)));
> > + return true;
> > + }
> Again, not sure why you're changing the indentation here.
>
> Generally the implementation looks good. You just need to eliminate the
> gratutious changes and fix the indentation/whitespace in your new code.
> I think it's basically ready to go with those fixes.
>
> Jeff
>
