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: Friday, October 21, 2022 2:54 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 Thu, Oct 20, 2022 at 07:34:13AM +0000, Jiang, Haochen wrote:
> > > > +  /* 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.
> 
> Please don't repeat historical mistakes.  You might not want to fix the
> existing code (since that can in theory break existing user code), but
> that is not a reason to punish users of a new feature as well ;-)
> 
> > > 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).
> 
> Oh huh, I looked for it but didn't find it.  Another argument for making
> better patch series ;-)
> 
> > 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.
> 
> You can use the data prefetch RTL code for all data loads just as well,
> it is more closely related than this -- but most people would call that
> insanity!

Maybe you got me here. I suppose I will write another patch for a new RTL to see
which implementation is better.

Thx,
Haochen

> 
> 
> Segher