On Thu, Oct 20, 2022 at 5:08 AM Segher Boessenkool
<seg...@kernel.crashing.org> wrote:
>
> 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
We'll split testcase into a separate patch.
> 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.
Yes, it's an inconvenience for review, sorry for that. But since we've
changed rtl def for prefetch, moving the general part into a separate
commit may break bootstrap when rtl-check is enabled.
-DEF_RTL_EXPR(PREFETCH, "prefetch", "eee", RTX_EXTRA)
+DEF_RTL_EXPR(PREFETCH, "prefetch", "eeee", RTX_EXTRA)
And we want to make each commit pass the bootstrap and regression test.
>
> Thanks,
>
>
> Segher
--
BR,
Hongtao
