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

[Segher Boessenkool <segher@kernel.crashing.org>]

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!


Segher