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

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

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

Thanks,


Segher