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

[Jiang, Haochen via Gcc-patches]

> -----Original Message-----
> From: Segher Boessenkool <seg...@kernel.crashing.org>
> Sent: Friday, October 21, 2022 2:54 AM
> To: Jiang, Haochen <haochen.ji...@intel.com>
> Cc: gcc-patches@gcc.gnu.org; rguent...@suse.de; Liu, Hongtao
> <hongtao....@intel.com>; ubiz...@gmail.com; richard.earns...@arm.com;
> richard.sandif...@arm.com; marcus.shawcr...@arm.com;
> kyrylo.tkac...@arm.com; r...@gcc.gnu.org; g...@amylaar.uk;
> claz...@synopsys.com; ni...@redhat.com; ramana.radhakrish...@arm.com;
> aol...@gcc.gnu.org; hubi...@ucw.cz; mfort...@gmail.com;
> dje....@gmail.com; li...@gcc.gnu.org; uweig...@de.ibm.com;
> kreb...@linux.ibm.com; olege...@gcc.gnu.org; da...@redhat.com;
> ebotca...@libertysurf.fr; jeffreya...@gmail.com; dave.ang...@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