Re: [PATCH 1/2] Add a parameter for the builtin function of prefetch to align with LLVM

[Hongtao Liu <crazylht@gmail.com>]

On Thu, Oct 20, 2022 at 9:39 AM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Thu, Oct 20, 2022 at 5:08 AM Segher Boessenkool
> <segher@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