Thank you @Jeff Law <jeffreya...@gmail.com> for your time and updated the
patch and reposted the changes.
Good Day,
~U
On Fri, Aug 15, 2025 at 8:11 AM Jeff Law <jeffreya...@gmail.com> wrote:
>
>
> On 7/16/25 10:54 PM, Umesh Kalappa wrote:
> > Fixed the regress that caused "pr118241.c" failure and tested "runtest
> --tool gcc
> --target_board='riscv-sim/-march=rv64gc_zba_zbb_zbc_zbs/-mabi=lp64/-mcmodel=medlow'
> riscv.exp" and 32 bit too
> >
> > lint warnings can be ignored for riscv-ext.opt.
> >
> > gcc/ChangeLog:
> >
> > * config/riscv/riscv-ext-mips.def (DEFINE_RISCV_EXT):
> > Added mips prefetch extension.
> > * config/riscv/riscv-ext.def: Likewise.
> > * config/riscv/riscv-ext.opt: Generated file.
> > * config/riscv/riscv.md (prefetch):.
> > Added mips prefetch address operand constraint.
> > * config/riscv/constraints.md: Added mips specific
> constraint.
> > * config/riscv/predicates.md (prefetch_operand):
> > Updated for mips 9 bits offset.
> > * config/riscv/riscv.cc (riscv_prefetch_offset_address_p):
> > Legitimate address with offset for prefetch check.
> > * config/riscv/riscv-protos.h: Likewise.
> > * config/riscv/riscv.h:
> > Macros to support for mips cached type.
> > * doc/riscv-ext.texi: Updated for mips prefetch.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/riscv/mipsprefetch.c: Test file for mips.pref.
> > ---
>
>
> > diff --git a/gcc/config/riscv/predicates.md
> b/gcc/config/riscv/predicates.md
> > index 1f9a6b562e5..ed7b34438c4 100644
> > --- a/gcc/config/riscv/predicates.md
> > +++ b/gcc/config/riscv/predicates.md
> > @@ -27,10 +27,14 @@
> > (ior (match_operand 0 "const_arith_operand")
> > (match_operand 0 "register_operand")))
> >
> > +(define_predicate "prefetch_const_operand"
> > + (and (match_code "const_int")
> > + (match_test "(IN_RANGE (INTVAL (op), 0, 511))")))
> > +
>
> [ ... ]
>
> >
> > +;; REG or REG+D where D fits in a uimm9
> > +(define_predicate "mips_prefetch_operand"
> > + (ior (match_operand 0 "register_operand")
> > + (and (match_test "prefetch_const_operand (op, VOIDmode)")
> > + (match_test "(IN_RANGE (INTVAL (XEXP (op, 1)), 0, 511))"))
> > + (and (match_code "plus")
> > + (match_test "register_operand (XEXP (op, 0), word_mode)")
> > + (match_test "prefetch_const_operand (XEXP (op, 1), VOIDmode)")
> > + (match_test "(IN_RANGE (INTVAL (XEXP (op, 1)), 0, 511))"))))
> So prefetch_const_operand tests the constant, so isn't the additional
> test in mips_prefetch_operand redundant?
>
>
>
>
> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > index 1275b034cf8..ef772659734 100644
> > --- a/gcc/config/riscv/riscv.cc
> > +++ b/gcc/config/riscv/riscv.cc
> > @@ -15389,6 +15389,34 @@ synthesize_and (rtx operands[3])
> > return true;
> > }
> >
> > +/* Given that we have an rtx of the form (prefetch ... WRITE LOCALITY),
> > + return the first operand of the associated PREF or PREFX insn. */
> > +rtx
> > +riscv_prefetch_cookie (rtx write, rtx locality)
> > +{
> > + /* store / load to data cache. */
> > + /* locality is unused. */
> > + return GEN_INT (INTVAL (write) + DCACHE_HINT + INTVAL (locality) * 0);
> Rather than a comment inside the function documenting the parameters,
> the standard way is to document them in the function comment before the
> function.
>
> Given the INTVAL (locality) * 0, why not just drop the term entirely?
> And if you want to keep the API for the function as-is, just drop the
> name of the second argument, but leave it's type.
>
>
>
>
> > +}
> > +
> > +/* Return true if X is a legitimate address with offset for prefetch.
> > + MODE is the mode of the value being accessed. */
> > +bool
> > +riscv_prefetch_offset_address_p (rtx x, machine_mode mode)
> > +{
> > + struct riscv_address_info addr;
> > +
> > + if (riscv_classify_address (&addr, x, mode, false)
> > + && addr.type == ADDRESS_REG)
> > + {
> > + if (TARGET_XMIPSCBOP)
> > + return CONST_INT_P (addr.offset)
> > + && MIPS_RISCV_9BIT_OFFSET_P (INTVAL (addr.offset));
> When you wrap, go ahead and add parenthesis so...
> (CONST_INT_P (addr.offset)
> && MIPS_RISCV_9BIT_OFFSET_P (INTVAL (addr.offset)));
>
>
>
> > diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
> > index 45fa521f219..270b3fafc7d 100644
> > --- a/gcc/config/riscv/riscv.h
> > +++ b/gcc/config/riscv/riscv.h
> > @@ -1319,4 +1319,13 @@ extern void
> riscv_remove_unneeded_save_restore_calls (void);
> >
> > #define TARGET_HAS_FMV_TARGET_ATTRIBUTE 0
> >
> > +/* mips pref valid offset range. */
> > +#define MIPS_RISCV_9BIT_OFFSET_P(OFFSET) (IN_RANGE (OFFSET, 0, 511))
> > +
> > +/* mips pref cache hint type. */
> > +#define ICACHE_HINT (0 << 3)
> > +#define DCACHE_HINT (1 << 3)
> > +#define SCACHE_HINT (2 << 3)
> > +#define TCACHE_HINT (3 << 3)
> Consider an enum; the primary advantage of an enum is they can be
> referenced symbolically by the debugger.
>
> None of those issues are show-stoppers. I think with them fixed this
> patch will be fine to go forward. Please update and repost.
>
> Thanks,
> jeff
>
