On Wed, Nov 27, 2024 at 8:50 PM Richard Biener <rguent...@suse.de> wrote:
>
> On Wed, 27 Nov 2024, Jakub Jelinek wrote:
>
> > Hi!
> >
> > The r15-4833-ge9ab41b79933 patch had among tons of config/i386
> > specific changes also important change to the generic code, allowing
> > also 2 as valid value of the second argument of __builtin_prefetch:
> > - /* Argument 1 must be either zero or one. */
> > - if (INTVAL (op1) != 0 && INTVAL (op1) != 1)
> > + /* Argument 1 must be 0, 1 or 2. */
> > + if (INTVAL (op1) < 0 || INTVAL (op1) > 2)
> >
> > But the patch failed to document that change in __builtin_prefetch
> > documentation, and more importantly didn't adjust any of the other
> > backends to deal with it (my understanding is the expected behavior
> > is that 2 will be silently handled as 0 unless backends have some
> > more specific way). Some of the backends would ICE on it, in some
> > cases gcc_assert failures/gcc_unreachable, in other cases crash later
> > (e.g. accessing arrays with that value as index and due to accessing
> > garbage after the array crashing at final.cc time), others treated 2
> > silently as 0, others treated 2 silently as 1.
> >
> > And even in the i386 backend there were bugs which caused ICEs.
> > The patch added some if (write == 0) and write 2 handling into
> > a (badly indented, maybe that is the reason, if (write == 1) body),
> > rather than into the else side, so it would be always false.
> >
> > The new *prefetch_rst2 define_insn only accepts parameters 2 1
> > (i.e. read-shared with moderate degree of locality), so in order
> > not to ICE the patch uses it only for __builtin_prefetch (ptr, 2, 1);
> > or __builtin_ia32_prefetch (ptr, 2, 1, 0); and not for other values
> > of the parameter. If that isn't what we want and we want it to be used
> > also for all or some of __builtin_prefetch (ptr, 2, {0,2,3}); and
> > corresponding __builtin_ia32_prefetch, maybe the define_insn could match
> > other values.
> > And there was another problem that -mno-mmx -mno-sse -mmovrs compilation
> > would ICE on most of the prefetches, so I had to add the FAIL; cases.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> OK, please leave other (target maintainers) time to comment.
LGTM for x86 part, thanks for handling this.
>
> Richard.
>
> > 2024-11-27 Jakub Jelinek <ja...@redhat.com>
> >
> > PR target/117608
> > * doc/extend.texi (__builtin_prefetch): Document that second
> > argument may be also 2 and its meaning.
> > * config/i386/i386.md (prefetch): Remove unreachable code.
> > Clear write set operands[1] to const0_rtx if !TARGET_MOVRS or
> > of locality is not 1. Formatting fixes.
> > * config/i386/i386-expand.cc (ix86_expand_builtin): Use IN_RANGE.
> > Call gen_prefetch even for TARGET_MOVRS.
> > * config/alpha/alpha.md (prefetch): Treat read_or_write 2 like 0.
> > * config/mips/mips.md (prefetch): Likewise.
> > * config/arc/arc.md (prefetch_1, prefetch_2, prefetch_3): Likewise.
> > * config/riscv/riscv.md (prefetch): Likewise.
> > * config/loongarch/loongarch.md (prefetch): Likewise.
> > * config/sparc/sparc.md (prefetch): Likewise. Use IN_RANGE.
> > * config/ia64/ia64.md (prefetch): Likewise.
> > * config/pa/pa.md (prefetch): Likewise.
> > * config/aarch64/aarch64.md (prefetch): Likewise.
> > * config/rs6000/rs6000.md (prefetch): Likewise.
> >
> > * gcc.dg/builtin-prefetch-1.c (good): Add tests with second argument
> > 2.
> > * gcc.target/i386/pr117608-1.c: New test.
> > * gcc.target/i386/pr117608-2.c: New test.
> >
> > --- gcc/doc/extend.texi.jj 2024-11-26 09:37:56.173574966 +0100
> > +++ gcc/doc/extend.texi 2024-11-26 17:49:35.469152396 +0100
> > @@ -15675,9 +15675,11 @@ be in the cache by the time it is access
> >
> > The value of @var{addr} is the address of the memory to prefetch.
> > There are two optional arguments, @var{rw} and @var{locality}.
> > -The value of @var{rw} is a compile-time constant one or zero; one
> > -means that the prefetch is preparing for a write to the memory address
> > -and zero, the default, means that the prefetch is preparing for a read.
> > +The value of @var{rw} is a compile-time constant zero, one or two; one
> > +means that the prefetch is preparing for a write to the memory address,
> > +two means that the prefetch is preparing for a shared read (expected to be
> > +read by at least one other processor before it is written if written at
> > +all) and zero, the default, means that the prefetch is preparing for a
> > read.
> > The value @var{locality} must be a compile-time constant integer between
> > zero and three. A value of zero means that the data has no temporal
> > locality, so it need not be left in the cache after the access. A value
> > --- gcc/config/i386/i386.md.jj 2024-11-26 09:37:56.047576735 +0100
> > +++ gcc/config/i386/i386.md 2024-11-26 18:18:34.662828702 +0100
> > @@ -28654,8 +28654,7 @@
> > [(prefetch (match_operand 0 "address_operand")
> > (match_operand:SI 1 "const_int_operand")
> > (match_operand:SI 2 "const_int_operand"))]
> > - "TARGET_3DNOW || TARGET_PREFETCH_SSE || TARGET_PRFCHW
> > - || TARGET_MOVRS"
> > + "TARGET_3DNOW || TARGET_PREFETCH_SSE || TARGET_PRFCHW || TARGET_MOVRS"
> > {
> > int write = INTVAL (operands[1]);
> > int locality = INTVAL (operands[2]);
> > @@ -28668,7 +28667,7 @@
> > SSE prefetch is not available (K6 machines). Otherwise use SSE
> > prefetch as it allows specifying of locality. */
> >
> > - if (write == 1)
> > + if (write == 1)
> > {
> > if (TARGET_PRFCHW)
> > operands[2] = GEN_INT (3);
> > @@ -28676,33 +28675,29 @@
> > operands[2] = GEN_INT (3);
> > else if (TARGET_PREFETCH_SSE)
> > operands[1] = const0_rtx;
> > - else if (write == 0)
> > - {
> > - gcc_assert (TARGET_3DNOW);
> > - operands[2] = GEN_INT (3);
> > - }
> > + else if (TARGET_3DNOW)
> > + operands[2] = GEN_INT (3);
> > else
> > {
> > - if (TARGET_MOVRS)
> > - ;
> > - else if (TARGET_PREFETCH_SSE)
> > - operands[1] = const0_rtx;
> > - else
> > - {
> > - gcc_assert (TARGET_3DNOW);
> > - operands[1] = const0_rtx;
> > - operands[2] = GEN_INT (3);
> > - }
> > + gcc_assert (TARGET_MOVRS);
> > + FAIL;
> > }
> > }
> > else
> > {
> > - if (TARGET_PREFETCH_SSE)
> > + if (!TARGET_MOVRS || locality != 1)
> > + {
> > + operands[1] = const0_rtx;
> > + write = 0;
> > + }
> > + if (TARGET_PREFETCH_SSE || write == 2)
> > ;
> > + else if (TARGET_3DNOW)
> > + operands[2] = GEN_INT (3);
> > else
> > {
> > - gcc_assert (TARGET_3DNOW);
> > - operands[2] = GEN_INT (3);
> > + gcc_assert (TARGET_MOVRS);
> > + FAIL;
> > }
> > }
> > })
> > --- gcc/config/i386/i386-expand.cc.jj 2024-11-26 09:37:56.019577128 +0100
> > +++ gcc/config/i386/i386-expand.cc 2024-11-26 18:03:04.095841152 +0100
> > @@ -14222,7 +14222,7 @@ ix86_expand_builtin (tree exp, rtx targe
> >
> > if (INTVAL (op3) == 1)
> > {
> > - if (INTVAL (op2) < 2 || INTVAL (op2) > 3)
> > + if (!IN_RANGE (INTVAL (op2), 2, 3))
> > {
> > error ("invalid third argument");
> > return const0_rtx;
> > @@ -14252,14 +14252,16 @@ ix86_expand_builtin (tree exp, rtx targe
> > op0 = copy_addr_to_reg (op0);
> > }
> >
> > - if (INTVAL (op2) < 0 || INTVAL (op2) > 3)
> > + if (!IN_RANGE (INTVAL (op2), 0, 3))
> > {
> > warning (0, "invalid third argument to
> > %<__builtin_ia32_prefetch%>; using zero");
> > op2 = const0_rtx;
> > }
> >
> > - if (TARGET_3DNOW || TARGET_PREFETCH_SSE
> > - || TARGET_PRFCHW)
> > + if (TARGET_3DNOW
> > + || TARGET_PREFETCH_SSE
> > + || TARGET_PRFCHW
> > + || TARGET_MOVRS)
> > emit_insn (gen_prefetch (op0, op1, op2));
> > else if (!MEM_P (op0) && side_effects_p (op0))
> > /* Don't do anything with direct references to volatile memory,
> > --- gcc/config/alpha/alpha.md.jj 2024-11-18 09:05:00.210283154 +0100
> > +++ gcc/config/alpha/alpha.md 2024-11-26 17:35:34.620915135 +0100
> > @@ -5171,7 +5171,7 @@ (define_insn "prefetch"
> > }
> > };
> >
> > - bool write = INTVAL (operands[1]) != 0;
> > + bool write = (INTVAL (operands[1]) & 1) != 0;
> > bool lru = INTVAL (operands[2]) != 0;
> >
> > return alt[write][lru];
> > --- gcc/config/mips/mips.md.jj 2024-09-03 16:48:01.609846676 +0200
> > +++ gcc/config/mips/mips.md 2024-11-26 17:40:24.016866697 +0100
> > @@ -7395,6 +7395,8 @@ (define_insn "prefetch"
> > else
> > return "lw\t$0,%a0";
> > }
> > + if (operands[1] == const2_rtx)
> > + operands[1] = const0_rtx;
> > /* Loongson ext2 implementation pref instructions. */
> > if (TARGET_LOONGSON_EXT2)
> > {
> > --- gcc/config/arc/arc.md.jj 2024-01-03 11:51:48.947489474 +0100
> > +++ gcc/config/arc/arc.md 2024-11-26 17:37:17.131481085 +0100
> > @@ -5439,9 +5439,9 @@ (define_insn "prefetch_1"
> > (match_operand:SI 2 "const_int_operand" "n"))]
> > "TARGET_HS"
> > {
> > - if (INTVAL (operands[1]))
> > + if ((INTVAL (operands[1]) & 1) != 0)
> > return "prefetchw [%0]";
> > - else
> > + else
> > return "prefetch [%0]";
> > }
> > [(set_attr "type" "load")
> > @@ -5454,9 +5454,9 @@ (define_insn "prefetch_2"
> > (match_operand:SI 3 "const_int_operand" "n,n,n"))]
> > "TARGET_HS"
> > {
> > - if (INTVAL (operands[2]))
> > + if ((INTVAL (operands[2]) & 1) != 0)
> > return "prefetchw\\t[%0, %1]";
> > - else
> > + else
> > return "prefetch\\t[%0, %1]";
> > }
> > [(set_attr "type" "load")
> > @@ -5468,10 +5468,10 @@ (define_insn "prefetch_3"
> > (match_operand:SI 2 "const_int_operand" "n"))]
> > "TARGET_HS"
> > {
> > - operands[0] = gen_rtx_MEM (SImode, operands[0]);
> > - if (INTVAL (operands[1]))
> > + operands[0] = gen_rtx_MEM (SImode, operands[0]);
> > + if ((INTVAL (operands[1]) & 1) != 0)
> > return "prefetchw%U0\\t%0";
> > - else
> > + else
> > return "prefetch%U0\\t%0";
> > }
> > [(set_attr "type" "load")
> > --- gcc/config/riscv/riscv.md.jj 2024-11-20 09:29:17.965421565 +0100
> > +++ gcc/config/riscv/riscv.md 2024-11-26 17:41:49.684668268 +0100
> > @@ -4219,7 +4219,8 @@ (define_insn "prefetch"
> > {
> > switch (INTVAL (operands[1]))
> > {
> > - case 0: return TARGET_ZIHINTNTL ? "%L2prefetch.r\t%a0" :
> > "prefetch.r\t%a0";
> > + case 0:
> > + case 2: return TARGET_ZIHINTNTL ? "%L2prefetch.r\t%a0" :
> > "prefetch.r\t%a0";
> > case 1: return TARGET_ZIHINTNTL ? "%L2prefetch.w\t%a0" :
> > "prefetch.w\t%a0";
> > default: gcc_unreachable ();
> > }
> > --- gcc/config/loongarch/loongarch.md.jj 2024-10-08 09:54:47.395484057
> > +0200
> > +++ gcc/config/loongarch/loongarch.md 2024-11-26 17:38:56.063097105 +0100
> > @@ -4091,7 +4091,8 @@ (define_insn "prefetch"
> > {
> > switch (INTVAL (operands[1]))
> > {
> > - case 0: return "preld\t0,%a0";
> > + case 0:
> > + case 2: return "preld\t0,%a0";
> > case 1: return "preld\t8,%a0";
> > default: gcc_unreachable ();
> > }
> > --- gcc/config/sparc/sparc.md.jj 2024-11-25 09:32:32.494140049 +0100
> > +++ gcc/config/sparc/sparc.md 2024-11-26 17:44:24.705499648 +0100
> > @@ -7832,9 +7832,9 @@ (define_insn "prefetch_64"
> > int read_or_write = INTVAL (operands[1]);
> > int locality = INTVAL (operands[2]);
> >
> > - gcc_assert (read_or_write == 0 || read_or_write == 1);
> > - gcc_assert (locality >= 0 && locality < 4);
> > - return prefetch_instr [read_or_write][locality == 0 ? 0 : 1];
> > + gcc_assert (IN_RANGE (read_or_write, 0, 2));
> > + gcc_assert (IN_RANGE (locality, 0, 3));
> > + return prefetch_instr [read_or_write & 1][locality == 0 ? 0 : 1];
> > }
> > [(set_attr "type" "load")
> > (set_attr "subtype" "prefetch")])
> > @@ -7858,9 +7858,9 @@ (define_insn "prefetch_32"
> > int read_or_write = INTVAL (operands[1]);
> > int locality = INTVAL (operands[2]);
> >
> > - gcc_assert (read_or_write == 0 || read_or_write == 1);
> > - gcc_assert (locality >= 0 && locality < 4);
> > - return prefetch_instr [read_or_write][locality == 0 ? 0 : 1];
> > + gcc_assert (IN_RANGE (read_or_write, 0, 2));
> > + gcc_assert (IN_RANGE (locality, 0, 3));
> > + return prefetch_instr [read_or_write & 1][locality == 0 ? 0 : 1];
> > }
> > [(set_attr "type" "load")
> > (set_attr "subtype" "prefetch")])
> > --- gcc/config/ia64/ia64.md.jj 2024-10-11 11:30:38.647392761 +0200
> > +++ gcc/config/ia64/ia64.md 2024-11-26 17:38:13.556691733 +0100
> > @@ -5041,9 +5041,9 @@ (define_insn "prefetch"
> > int i = (INTVAL (operands[1]));
> > int j = (INTVAL (operands[2]));
> >
> > - gcc_assert (i == 0 || i == 1);
> > - gcc_assert (j >= 0 && j <= 3);
> > - return alt[i][j];
> > + gcc_assert (IN_RANGE (i, 0, 2));
> > + gcc_assert (IN_RANGE (j, 0, 3));
> > + return alt[i & 1][j];
> > }
> > [(set_attr "itanium_class" "lfetch")])
> >
> > --- gcc/config/pa/pa.md.jj 2024-11-26 09:37:56.074576356 +0100
> > +++ gcc/config/pa/pa.md 2024-11-26 17:41:11.558201627 +0100
> > @@ -10354,7 +10354,7 @@ (define_insn "prefetch_20"
> > "ldd 0(%0),%%r0"
> > }
> > };
> > - int read_or_write = INTVAL (operands[1]) == 0 ? 0 : 1;
> > + int read_or_write = INTVAL (operands[1]) & 1;
> > int locality = INTVAL (operands[2]) == 0 ? 0 : 1;
> >
> > return instr [locality][read_or_write];
> > --- gcc/config/aarch64/aarch64.md.jj 2024-11-21 09:32:35.264366968 +0100
> > +++ gcc/config/aarch64/aarch64.md 2024-11-26 17:34:29.648824047 +0100
> > @@ -1020,7 +1020,7 @@ (define_insn "prefetch"
> > the address into a DImode MEM so that aarch64_print_operand knows
> > how to print it. */
> > operands[0] = gen_rtx_MEM (DImode, operands[0]);
> > - return pftype[INTVAL(operands[1])][locality];
> > + return pftype[INTVAL (operands[1]) & 1][locality];
> > }
> > [(set_attr "type" "load_4")]
> > )
> > --- gcc/config/rs6000/rs6000.md.jj 2024-11-21 09:32:35.411364879 +0100
> > +++ gcc/config/rs6000/rs6000.md 2024-11-26 17:42:30.845092465 +0100
> > @@ -14353,14 +14353,14 @@ (define_insn "prefetch"
> >
> > if (REG_P (operands[0]))
> > {
> > - if (INTVAL (operands[1]) == 0)
> > + if (INTVAL (operands[1]) != 1)
> > return inst_select ? "dcbt 0,%0" : "dcbt 0,%0,16";
> > else
> > return inst_select ? "dcbtst 0,%0" : "dcbtst 0,%0,16";
> > }
> > else
> > {
> > - if (INTVAL (operands[1]) == 0)
> > + if (INTVAL (operands[1]) != 1)
> > return inst_select ? "dcbt %a0" : "dcbt %a0,16";
> > else
> > return inst_select ? "dcbtst %a0" : "dcbtst %a0,16";
> > --- gcc/testsuite/gcc.dg/builtin-prefetch-1.c.jj 2024-11-01
> > 19:32:19.177964137 +0100
> > +++ gcc/testsuite/gcc.dg/builtin-prefetch-1.c 2024-11-26 17:56:00.493766587
> > +0100
> > @@ -23,6 +23,10 @@ good (int *p)
> > __builtin_prefetch (p, 1, 1);
> > __builtin_prefetch (p, 1, 2);
> > __builtin_prefetch (p, 1, 3);
> > + __builtin_prefetch (p, 2, 0);
> > + __builtin_prefetch (p, 2, 1);
> > + __builtin_prefetch (p, 2, 2);
> > + __builtin_prefetch (p, 2, 3);
> > }
> >
> > void
> > --- gcc/testsuite/gcc.target/i386/pr117608-1.c.jj 2024-11-26
> > 18:03:34.775411999 +0100
> > +++ gcc/testsuite/gcc.target/i386/pr117608-1.c 2024-11-26
> > 18:22:08.573837662 +0100
> > @@ -0,0 +1,14 @@
> > +/* PR target/117608 */
> > +/* { dg-do compile } */
> > +/* { dg-options "-mno-movrs" } */
> > +
> > +int i;
> > +
> > +void
> > +foo (void)
> > +{
> > + __builtin_ia32_prefetch (&i, 2, 0, 0);
> > + __builtin_ia32_prefetch (&i, 2, 1, 0);
> > + __builtin_ia32_prefetch (&i, 2, 2, 0);
> > + __builtin_ia32_prefetch (&i, 2, 3, 0);
> > +}
> > --- gcc/testsuite/gcc.target/i386/pr117608-2.c.jj 2024-11-26
> > 18:20:47.385972882 +0100
> > +++ gcc/testsuite/gcc.target/i386/pr117608-2.c 2024-11-26
> > 18:21:27.995405054 +0100
> > @@ -0,0 +1,14 @@
> > +/* PR target/117608 */
> > +/* { dg-do compile } */
> > +/* { dg-options "-mno-mmx -mno-sse -mmovrs" } */
> > +
> > +int i;
> > +
> > +void
> > +foo (void)
> > +{
> > + __builtin_ia32_prefetch (&i, 2, 0, 0);
> > + __builtin_ia32_prefetch (&i, 2, 1, 0);
> > + __builtin_ia32_prefetch (&i, 2, 2, 0);
> > + __builtin_ia32_prefetch (&i, 2, 3, 0);
> > +}
> >
> > Jakub
> >
> >
>
> --
> Richard Biener <rguent...@suse.de>
> SUSE Software Solutions Germany GmbH,
> Frankenstrasse 146, 90461 Nuernberg, Germany;
> GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
--
BR,
Hongtao
