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

["Jiang, Haochen" <haochen.jiang@intel.com>]

> -----Original Message-----
> From: Segher Boessenkool <segher@kernel.crashing.org>
> Sent: Thursday, October 20, 2022 5:07 AM
> To: Jiang, Haochen <haochen.jiang@intel.com>
> Cc: gcc-patches@gcc.gnu.org; rguenther@suse.de; Liu, Hongtao
> <hongtao.liu@intel.com>; ubizjak@gmail.com; richard.earnshaw@arm.com;
> richard.sandiford@arm.com; marcus.shawcroft@arm.com;
> kyrylo.tkachov@arm.com; rth@gcc.gnu.org; gnu@amylaar.uk;
> claziss@synopsys.com; nickc@redhat.com; ramana.radhakrishnan@arm.com;
> aoliva@gcc.gnu.org; hubicka@ucw.cz; mfortune@gmail.com;
> dje.gcc@gmail.com; linkw@gcc.gnu.org; uweigand@de.ibm.com;
> krebbel@linux.ibm.com; olegendo@gcc.gnu.org; davem@redhat.com;
> ebotcazou@libertysurf.fr; jeffreyalaw@gmail.com; dave.anglin@bell.net
> Subject: Re: [PATCH 1/2] Add a parameter for the builtin function of prefetch
> to align with LLVM
> 
> 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).

I will change that in next patch.

> 
> > +  /* 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?

Will change to 1 to avoid confusion in that. The reason why this is a warning
is because previous ones related to constant arguments out of range in prefetch
are also using warning.

/* Argument 2 must be 0, 1, 2, or 3.  */
  if (INTVAL (op2) < 0 || INTVAL (op2) > 3)
    {
      warning (0, "invalid third argument to %<__builtin_prefetch%>; using zero");
      op2 = const0_rtx;
    }

Therefore I use warning to align with them.

> 
> > --- 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.

I will fix that in updated patch.

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

Actually I changed documentation for prefetch but it is flooded in the patch
(Sorry for that).

In gcc/doc/rtl.texi

-@item (prefetch:@var{m} @var{addr} @var{rw} @var{locality})
+@item (prefetch:@var{m} @var{addr} @var{rw} @var{locality} @var{cache})
 
+Operand @var{cache} is 1 if the prefetch is prefetching data, 0 for prefetching
+instruction;
+targets that do not support instruction prefetch should treat all as data
+prefetch.
 
And for the implementation on the instruction prefetch, actually I have thought
of that way previously. But I chose the way how patch current goes for the
following reasons.

1. Previously we are using parameter to indicate r/w and locality in prefetch. I
suppose it is quite similar in this case. Since the pattern is already there, I prefer
reusing them.

2. It will be more natural for developers to extend their prefetch in future.

If anyone have points, welcome further discussion on that.

> When you send an updated patch, please split it up better?  Generic
> changes and documentation in one patch, target changes in a separate
> 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.

Really sorry for that. Hongtao has explained the reason for why we arrange
this patch and I will split the testcase to another patch.

Also if the change on testsuites on this patch change to minimal change,
the patch will be much smaller than current one.

BRs,
Haochen

> 
> Thanks,
> 
> 
> Segher