On Thu, Oct 20, 2022 at 9:39 AM Hongtao Liu <crazy...@gmail.com> wrote:
>
> On Thu, Oct 20, 2022 at 5:08 AM Segher Boessenkool
> <seg...@kernel.crashing.org> wrote:
> >
> > On Fri, Oct 14, 2022 at 04:34:05PM +0800, Haochen Jiang wrote:
> > > * config/s390/s390.cc (s390_expand_cpymem): Generate fourth
> > > parameter for
> >
> > (Many too long lines here, this is the first one. Changelog lines are
> > max. 80 positions; a tab is eight).
> >
> > > + /* Argument 3 must be either zero or one. */
> > > + if (INTVAL (op3) != 0 && INTVAL (op3) != 1)
> > > + {
> > > + warning (0, "invalid fourth argument to %<__builtin_prefetch%>;"
> > > + " using one");
> >
> > "using 1" makes sense maybe, but "using one" reads as "using an
> > argument", not very sane.
> >
> > An error would be better here anyway?
> >
> > > --- a/gcc/config/rs6000/rs6000.md
> > > +++ b/gcc/config/rs6000/rs6000.md
> > > @@ -14060,10 +14060,25 @@
> > > DONE;
> > > })
> > >
> > > -(define_insn "prefetch"
> > > +(define_expand "prefetch"
> > > + [(prefetch (match_operand 0 "indexed_or_indirect_address")
> > > + (match_operand:SI 1 "const_int_operand")
> > > + (match_operand:SI 2 "const_int_operand")
> > > + (match_operand:SI 3 "const_int_operand"))]
> > > + ""
> > > +{
> > > + if (INTVAL (operands[3]) == 0)
> > > + {
> >
> > Broken indentation.
> >
> > > + warning (0, "instruction prefetch is not supported; using data
> > > prefetch");
> >
> > Please use a separate pattern for this, and leave prefetch to mean data
> > prefetch, as documented! Documentation you didn't change btw. Call the
> > new one instruction_prefetch or something equally boring maybe :-)
Yes, Maybe we should add new rtl def named "iprefetch", so there will
be no need to change other backend.
> >
> > When you send an updated patch, please split it up better? Generic
> > changes and documentation in one patch, target changes in a separate
> We'll split testcase into a separate patch.
> > patch or patches, and testsuite is distinct as well. It isn't nice to
> > have to scroll through thousands of lines to see if there is anything
> > relevant to you.
>
> Yes, it's an inconvenience for review, sorry for that. But since we've
> changed rtl def for prefetch, moving the general part into a separate
> commit may break bootstrap when rtl-check is enabled.
> -DEF_RTL_EXPR(PREFETCH, "prefetch", "eee", RTX_EXTRA)
> +DEF_RTL_EXPR(PREFETCH, "prefetch", "eeee", RTX_EXTRA)
>
> And we want to make each commit pass the bootstrap and regression test.
> >
> > Thanks,
> >
> >
> > Segher
>
>
>
> --
> BR,
> Hongtao
--
BR,
Hongtao
