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 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 :-)
>
> 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