[PATCH 0/2] RISC-V: Define not broken prefetch builtins

[PATCH 0/2] RISC-V: Define not broken prefetch builtins

[Tsukasa OI]

Hello,

As I explained earlier:
<https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626916.html>,
the builtin function for RISC-V "__builtin_riscv_zicbop_cbo_prefetchi" is
completely broken.  Instead, this patch set (in PATCH 1/2) creates three
new, working builtin intrinsics.

void __builtin_riscv_prefetch_i(void *addr, [intptr_t offset,] ...);
void __builtin_riscv_prefetch_r(void *addr, [intptr_t offset,] ...);
void __builtin_riscv_prefetch_w(void *addr, [intptr_t offset,] ...);


For consistency with "prefetch.i" and the reason I describe later (which
requires native instructions for "prefetch.r" and "prefetch.w"), I decided
to make builtin functions for "prefetch.[rw]" as well.

Optional second argument (named "offset" here) defaults to zero and must be
a compile-time integral constant.  Also, it must be a valid offset for a
"prefetch.[irw]" HINT instruction (x % 32 == 0 && x >= -2048 && x < 2048).

They are defined if the 'Zicbop' extension is supported and expands to:

> prefetch.i offset(addr_reg)  ; __builtin_riscv_prefetch_i
> prefetch.r offset(addr_reg)  ; __builtin_riscv_prefetch_r
> prefetch.w offset(addr_reg)  ; __builtin_riscv_prefetch_w


The hardest part of this patch set was to support builtin function with
variable argument (making "offset" optional).  It required:

1.  Support for variable argument function prototype for RISC-V builtins
    (corresponding "..." on C-based languages)
2.  Support for (non-vector) RISC-V builtins with custom expansion
    (on RVV intrinsics, custom expansion is already implemented)


... and PATCH 2/2 fixes an ICE while I'm investigating regular prefetch
builtin (__builtin_prefetch).  If the 'Zicbop' extension is enabled,
__builtin_prefetch with the first argument NULL or (not all but) some
fixed addresses (like ((void*)0x20)) can cause an ICE.  This is because
the "r" constraint is not checked and a constant can be a first argument
of target-specific "prefetch" RTL instruction.

PATCH 2/2 fixes this issue by:

1.  Making "prefetch" not an instruction but instead an expansion
    (this is not rare; e.g. on i386) and
2.  Coercing the address argument into a register in the expansion

It requires separate instructions for "prefetch.[rw]" and I decided to make
those prefetch instructions very similar to "prefetch.i".  That's one of the
reasons I created builtins corresponding those.


Sincerely,
Tsukasa




Tsukasa OI (2):
  RISC-V: Define not broken prefetch builtins
  RISC-V: Fix ICE by expansion and register coercion

 gcc/config/riscv/riscv-builtins.cc            | 112 +++++++++++++++++-
 gcc/config/riscv/riscv-cmo.def                |   8 +-
 gcc/config/riscv/riscv-ftypes.def             |   1 +
 gcc/config/riscv/riscv.md                     |  67 ++++++++---
 gcc/testsuite/gcc.target/riscv/cmo-zicbop-1.c |  41 ++++---
 gcc/testsuite/gcc.target/riscv/cmo-zicbop-2.c |  33 ++----
 gcc/testsuite/gcc.target/riscv/cmo-zicbop-3.c |  29 +++++
 gcc/testsuite/gcc.target/riscv/cmo-zicbop-4.c |  14 +++
 gcc/testsuite/gcc.target/riscv/cmo-zicbop-5.c |  14 +++
 gcc/testsuite/gcc.target/riscv/cmo-zicbop-6.c |  38 ++++++
 .../gcc.target/riscv/cmo-zicbop-by-common-1.c |  17 +++
 .../gcc.target/riscv/cmo-zicbop-by-common-2.c |   7 ++
 .../gcc.target/riscv/cmo-zicbop-by-common-3.c |  13 ++
 .../riscv/cmo-zicbop-by-common-ice-1.c        |  13 ++
 .../riscv/cmo-zicbop-by-common-ice-2.c        |   7 ++
 15 files changed, 350 insertions(+), 64 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-3.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-4.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-5.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-6.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-2.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-3.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-ice-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-ice-2.c


base-commit: 40ac613627205dd4d24ae136917e48b357fee758
-- 
2.42.0

[PATCH 1/2] RISC-V: Define not broken prefetch builtins

[Tsukasa OI]

From: Tsukasa OI <research_trasio@irq.a4lg.com>

__builtin_riscv_zicbop_cbo_prefetchi (corresponding "prefetch.i"
instruction from the 'Zicbop' extension) is completely broken and new
builtin is required for replacement.

However, it required more than defining new builtin and/or instruction.

1.  Support for variable argument function prototype for RISC-V builtins
    (corresponding "..." on C-based languages)
2.  Support for (non-vector) RISC-V builtins with custom expansion
    (on RVV intrinsics, custom expansion is already implemented)

Along with other minor changes, not broken "prefetch.i" intrinsic is
defined as follows:

    void __builtin_riscv_prefetch_i (void *addr, ...);

Optional second argument (defaults to zero and must be a compile-time
constant integer) is the offset from given address (-2048 <= x < 2048 and
must be a multiple of 32, due to "prefetch.i" constraints).  Third or later
arguments are ignored (like other builtin functions).

This commit also defines builtin functions for "prefetch.r" and "prefetch.w"
instructions for consistency:

    void __builtin_riscv_prefetch_r (void *addr, ...);
    void __builtin_riscv_prefetch_w (void *addr, ...);

Those instructions can be emitted using __builtin_prefetch but has no
control of the offset field.

gcc/ChangeLog:

	* config/riscv/riscv-builtins.cc: Rename availabilities
	"prefetchi{32,64}" to "prefetch{32,64}".
	(RISCV_FTYPE_NAME_VAR1): Similar to RISCV_FTYPE_NAME1 but for
	variable argument function prototype.
	(DEF_RISCV_FTYPE_VAR): Similar to DEF_RISCV_FTYPE but calls
	RISCV_FTYPE_NAME_VAR* instead.
	(enum riscv_builtin_type): Add RISCV_BUILTIN_CUSTOM for builtin
	with custom expansion.
	(struct riscv_builtin_description): Add custom expansion function.
	(RISCV_BUILTIN): Modified to set "expand_function".
	(RISCV_CUSTOM_BUILTIN): New.  Similar to RISCV_BUILTIN but only for
	builtins with custom expansion function.
	(riscv_expand_builtin): Handle RISCV_BUILTIN_CUSTOM builtin.
	(expand_builtin_prefetch_riscv): New custom expansion function for
	"prefetch.[irw]" instructions from the 'Zicbop' extension.
	* config/riscv/riscv-cmo.def
	(__builtin_riscv_zicbop_cbo_prefetchi): Remove since it's broken.
	(__builtin_riscv_prefetch_i): New.
	(__builtin_riscv_prefetch_r): New.
	(__builtin_riscv_prefetch_w): New.
	* config/riscv/riscv-ftypes.def: Add variable argument prototype
	for "void func(void*, ...)".
	* config/riscv/riscv.md (unspecv): Remove UNSPECV_PREI and add
	UNSPECV_PREFETCH_[IRW].
	(riscv_prefetchi_<mode>): Remove.
	(riscv_prefetch_i_<mode>): New.
	(riscv_prefetch_r_<mode>): New.
	(riscv_prefetch_w_<mode>): New.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/cmo-zicbop-1.c: Refine to test new builtins.
	* gcc.target/riscv/cmo-zicbop-2.c: Ditto.
	* gcc.target/riscv/cmo-zicbop-3.c: New NULL prefetching test.
	* gcc.target/riscv/cmo-zicbop-4.c: New failure test.
	* gcc.target/riscv/cmo-zicbop-5.c: Ditto.
	* gcc.target/riscv/cmo-zicbop-6.c: Ditto.
	* gcc.target/riscv/cmo-zicbop-by-common-1.c: New test for
	__builtin_prefetch and the 'Zicbop' extension.
	* gcc.target/riscv/cmo-zicbop-by-common-2.c: Ditto.
	* gcc.target/riscv/cmo-zicbop-by-common-3.c: Ditto.
---
 gcc/config/riscv/riscv-builtins.cc            | 112 +++++++++++++++++-
 gcc/config/riscv/riscv-cmo.def                |   8 +-
 gcc/config/riscv/riscv-ftypes.def             |   1 +
 gcc/config/riscv/riscv.md                     |  30 ++++-
 gcc/testsuite/gcc.target/riscv/cmo-zicbop-1.c |  41 ++++---
 gcc/testsuite/gcc.target/riscv/cmo-zicbop-2.c |  33 ++----
 gcc/testsuite/gcc.target/riscv/cmo-zicbop-3.c |  29 +++++
 gcc/testsuite/gcc.target/riscv/cmo-zicbop-4.c |  14 +++
 gcc/testsuite/gcc.target/riscv/cmo-zicbop-5.c |  14 +++
 gcc/testsuite/gcc.target/riscv/cmo-zicbop-6.c |  38 ++++++
 .../gcc.target/riscv/cmo-zicbop-by-common-1.c |  17 +++
 .../gcc.target/riscv/cmo-zicbop-by-common-2.c |   7 ++
 .../gcc.target/riscv/cmo-zicbop-by-common-3.c |  13 ++
 13 files changed, 305 insertions(+), 52 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-3.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-4.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-5.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-6.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-2.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-3.c

diff --git a/gcc/config/riscv/riscv-builtins.cc b/gcc/config/riscv/riscv-builtins.cc
index 3fe3a89dcc25..4f422e0891c2 100644
--- a/gcc/config/riscv/riscv-builtins.cc
+++ b/gcc/config/riscv/riscv-builtins.cc
@@ -47,12 +47,16 @@ along with GCC; see the file COPYING3.  If not see
 #define RISCV_FTYPE_NAME1(A, B) RISCV_##A##_FTYPE_##B
 #define RISCV_FTYPE_NAME2(A, B, C) RISCV_##A##_FTYPE_##B##_##C
 #define RISCV_FTYPE_NAME3(A, B, C, D) RISCV_##A##_FTYPE_##B##_##C##_##D
+/* Similar, but for variable argument function prototype.  */
+#define RISCV_FTYPE_NAME_VAR1(A, B) RISCV_##A##_FTYPE_##B##_VAR
 
 /* Classifies the prototype of a built-in function.  */
 enum riscv_function_type {
 #define DEF_RISCV_FTYPE(NARGS, LIST) RISCV_FTYPE_NAME##NARGS LIST,
+#define DEF_RISCV_FTYPE_VAR(NARGS, LIST) RISCV_FTYPE_NAME_VAR##NARGS LIST,
 #include "config/riscv/riscv-ftypes.def"
 #undef DEF_RISCV_FTYPE
+#undef DEF_RISCV_FTYPE_VAR
   RISCV_MAX_FTYPE_MAX
 };
 
@@ -62,7 +66,10 @@ enum riscv_builtin_type {
   RISCV_BUILTIN_DIRECT,
 
   /* Likewise, but with return type VOID.  */
-  RISCV_BUILTIN_DIRECT_NO_TARGET
+  RISCV_BUILTIN_DIRECT_NO_TARGET,
+
+  /* The function which requires custom expansion.  */
+  RISCV_BUILTIN_CUSTOM,
 };
 
 /* Declare an availability predicate for built-in functions.  */
@@ -90,6 +97,9 @@ struct riscv_builtin_description {
 
   /* Whether the function is available.  */
   unsigned int (*avail) (void);
+
+  /* Custom expansion function if builtin_type is RISCV_BUILTIN_CUSTOM.  */
+  rtx (*expand_function) (enum insn_code icode, rtx target, tree exp);
 };
 
 AVAIL (hard_float, TARGET_HARD_FLOAT || TARGET_ZFINX)
@@ -101,8 +111,8 @@ AVAIL (inval32, TARGET_ZICBOM && !TARGET_64BIT)
 AVAIL (inval64, TARGET_ZICBOM && TARGET_64BIT)
 AVAIL (zero32,  TARGET_ZICBOZ && !TARGET_64BIT)
 AVAIL (zero64,  TARGET_ZICBOZ && TARGET_64BIT)
-AVAIL (prefetchi32, TARGET_ZICBOP && !TARGET_64BIT)
-AVAIL (prefetchi64, TARGET_ZICBOP && TARGET_64BIT)
+AVAIL (prefetch32, TARGET_ZICBOP && !TARGET_64BIT)
+AVAIL (prefetch64, TARGET_ZICBOP && TARGET_64BIT)
 AVAIL (crypto_zbkb32, TARGET_ZBKB && !TARGET_64BIT)
 AVAIL (crypto_zbkb64, TARGET_ZBKB && TARGET_64BIT)
 AVAIL (crypto_zbkx32, TARGET_ZBKX && !TARGET_64BIT)
@@ -123,7 +133,7 @@ AVAIL (clmulr_zbc32, TARGET_ZBC && !TARGET_64BIT)
 AVAIL (clmulr_zbc64, TARGET_ZBC && TARGET_64BIT)
 AVAIL (hint_pause, (!0))
 
-/* Construct a riscv_builtin_description from the given arguments.
+/* Construct a simple riscv_builtin_description from the given arguments.
 
    INSN is the name of the associated instruction pattern, without the
    leading CODE_FOR_riscv_.
@@ -137,7 +147,7 @@ AVAIL (hint_pause, (!0))
    riscv_builtin_avail_.  */
 #define RISCV_BUILTIN(INSN, NAME, BUILTIN_TYPE,	FUNCTION_TYPE, AVAIL)	\
   { CODE_FOR_riscv_ ## INSN, "__builtin_riscv_" NAME,			\
-    BUILTIN_TYPE, FUNCTION_TYPE, riscv_builtin_avail_ ## AVAIL }
+    BUILTIN_TYPE, FUNCTION_TYPE, riscv_builtin_avail_ ## AVAIL, NULL }
 
 /* Define __builtin_riscv_<INSN>, which is a RISCV_BUILTIN_DIRECT function
    mapped to instruction CODE_FOR_riscv_<INSN>,  FUNCTION_TYPE and AVAIL
@@ -152,6 +162,25 @@ AVAIL (hint_pause, (!0))
   RISCV_BUILTIN (INSN, #INSN, RISCV_BUILTIN_DIRECT_NO_TARGET,		\
 		FUNCTION_TYPE, AVAIL)
 
+/* Construct a custom riscv_builtin_description from the given arguments.
+
+   INSN is the name of the associated instruction pattern, without the
+   leading CODE_FOR_riscv_.
+
+   NAME is the name of the function itself, without the leading
+   "__builtin_riscv_".
+
+   FTYPE is the prototype field in riscv_builtin_description.
+
+   AVAIL is the name of the availability predicate, without the leading
+   riscv_builtin_avail_.
+
+   EXPANDER is the function to expand the builtin.  */
+#define RISCV_CUSTOM_BUILTIN(INSN, NAME, FTYPE, AVAIL, EXPANDER)	\
+  { CODE_FOR_riscv_ ## INSN, "__builtin_riscv_" NAME,			\
+    RISCV_BUILTIN_CUSTOM, FTYPE,					\
+    riscv_builtin_avail_ ## AVAIL, EXPANDER }
+
 /* Argument types.  */
 #define RISCV_ATYPE_VOID void_type_node
 #define RISCV_ATYPE_UQI unsigned_intQI_type_node
@@ -171,6 +200,8 @@ AVAIL (hint_pause, (!0))
 #define RISCV_FTYPE_ATYPES3(A, B, C, D) \
   RISCV_ATYPE_##A, RISCV_ATYPE_##B, RISCV_ATYPE_##C, RISCV_ATYPE_##D
 
+static rtx expand_builtin_prefetch_riscv (enum insn_code icode, rtx target, tree exp);
+
 static const struct riscv_builtin_description riscv_builtins[] = {
   #include "riscv-cmo.def"
   #include "riscv-scalar-crypto.def"
@@ -209,8 +240,15 @@ riscv_build_function_type (enum riscv_function_type type)
       = build_function_type_list (RISCV_FTYPE_ATYPES##NUM ARGS,		\
 				  NULL_TREE);				\
     break;
+#define DEF_RISCV_FTYPE_VAR(NUM, ARGS)					\
+  case RISCV_FTYPE_NAME_VAR##NUM ARGS:				\
+    types[(int) type]							\
+      = build_varargs_function_type_list (RISCV_FTYPE_ATYPES##NUM ARGS,	\
+					  NULL_TREE);			\
+    break;
 #include "config/riscv/riscv-ftypes.def"
 #undef DEF_RISCV_FTYPE
+#undef DEF_RISCV_FTYPE_VAR
       default:
 	gcc_unreachable ();
       }
@@ -387,6 +425,9 @@ riscv_expand_builtin (tree exp, rtx target, rtx subtarget ATTRIBUTE_UNUSED,
 
 	  case RISCV_BUILTIN_DIRECT_NO_TARGET:
 	    return riscv_expand_builtin_direct (d->icode, target, exp, false);
+
+	  case RISCV_BUILTIN_CUSTOM:
+	    return d->expand_function (d->icode, target, exp);
 	  }
       }
     }
@@ -394,6 +435,67 @@ riscv_expand_builtin (tree exp, rtx target, rtx subtarget ATTRIBUTE_UNUSED,
   gcc_unreachable ();
 }
 
+static rtx
+expand_builtin_prefetch_riscv (enum insn_code icode,
+			       rtx target ATTRIBUTE_UNUSED,
+			       tree exp)
+{
+  struct expand_operand ops[2];
+  tree arg0, arg1;
+  rtx op1;
+
+  char name_suffix = 'i';
+  switch (icode)
+    {
+    case CODE_FOR_riscv_prefetch_i_si:
+    case CODE_FOR_riscv_prefetch_i_di:
+      break;
+    case CODE_FOR_riscv_prefetch_r_si:
+    case CODE_FOR_riscv_prefetch_r_di:
+      name_suffix = 'r';
+      break;
+    case CODE_FOR_riscv_prefetch_w_si:
+    case CODE_FOR_riscv_prefetch_w_di:
+      name_suffix = 'w';
+      break;
+    default:
+      gcc_unreachable ();
+    }
+
+  /* Argument 0 is an address.  */
+  arg0 = CALL_EXPR_ARG (exp, 0);
+
+  /* Argument 1 (offset) is an optional compile-time constant and defaults
+     to zero.  It must be a valid offset for a S-type instruction AND must be
+     a multiple of 32.  */
+  if (call_expr_nargs (exp) > 1)
+    arg1 = CALL_EXPR_ARG (exp, 1);
+  else
+    arg1 = integer_zero_node;
+  if (TREE_CODE (arg1) != INTEGER_CST)
+    {
+      error ("second argument to %<__builtin_riscv_prefetch_%c%> must be a"
+	     " constant", name_suffix);
+      arg1 = integer_zero_node;
+    }
+  op1 = expand_normal (arg1);
+  if (INTVAL (op1) < -IMM_REACH / 2
+      || INTVAL (op1) >= IMM_REACH / 2
+      || INTVAL (op1) % 32 != 0)
+    {
+      error ("second argument to %<__builtin_riscv_prefetch_%c%> must be a"
+	     " valid offset for the prefetch instruction", name_suffix);
+      arg1 = integer_zero_node;
+      op1 = expand_normal (arg1);
+    }
+
+  create_input_operand (&ops[0], expand_normal (arg0),
+			TYPE_MODE (TREE_TYPE (arg0)));
+  create_input_operand (&ops[1], op1,
+			TYPE_MODE (TREE_TYPE (arg1)));
+  return riscv_expand_builtin_insn (icode, 2, ops, false);
+}
+
 /* Implement TARGET_ATOMIC_ASSIGN_EXPAND_FENV.  */
 
 void
diff --git a/gcc/config/riscv/riscv-cmo.def b/gcc/config/riscv/riscv-cmo.def
index ff713b78e19e..3be77710095e 100644
--- a/gcc/config/riscv/riscv-cmo.def
+++ b/gcc/config/riscv/riscv-cmo.def
@@ -13,8 +13,12 @@ RISCV_BUILTIN (zero_si, "zicboz_cbo_zero", RISCV_BUILTIN_DIRECT_NO_TARGET, RISCV
 RISCV_BUILTIN (zero_di, "zicboz_cbo_zero", RISCV_BUILTIN_DIRECT_NO_TARGET, RISCV_VOID_FTYPE_VOID_PTR, zero64),
 
 // zicbop
-RISCV_BUILTIN (prefetchi_si, "zicbop_cbo_prefetchi", RISCV_BUILTIN_DIRECT, RISCV_USI_FTYPE_USI, prefetchi32),
-RISCV_BUILTIN (prefetchi_di, "zicbop_cbo_prefetchi", RISCV_BUILTIN_DIRECT, RISCV_UDI_FTYPE_UDI, prefetchi64),
+RISCV_CUSTOM_BUILTIN (prefetch_i_si, "prefetch_i", RISCV_VOID_FTYPE_VOID_PTR_VAR, prefetch32, expand_builtin_prefetch_riscv),
+RISCV_CUSTOM_BUILTIN (prefetch_i_di, "prefetch_i", RISCV_VOID_FTYPE_VOID_PTR_VAR, prefetch64, expand_builtin_prefetch_riscv),
+RISCV_CUSTOM_BUILTIN (prefetch_r_si, "prefetch_r", RISCV_VOID_FTYPE_VOID_PTR_VAR, prefetch32, expand_builtin_prefetch_riscv),
+RISCV_CUSTOM_BUILTIN (prefetch_r_di, "prefetch_r", RISCV_VOID_FTYPE_VOID_PTR_VAR, prefetch64, expand_builtin_prefetch_riscv),
+RISCV_CUSTOM_BUILTIN (prefetch_w_si, "prefetch_w", RISCV_VOID_FTYPE_VOID_PTR_VAR, prefetch32, expand_builtin_prefetch_riscv),
+RISCV_CUSTOM_BUILTIN (prefetch_w_di, "prefetch_w", RISCV_VOID_FTYPE_VOID_PTR_VAR, prefetch64, expand_builtin_prefetch_riscv),
 
 // zbkc or zbc
 RISCV_BUILTIN (clmul_si, "clmul", RISCV_BUILTIN_DIRECT, RISCV_USI_FTYPE_USI_USI, clmul_zbkc32_or_zbc32),
diff --git a/gcc/config/riscv/riscv-ftypes.def b/gcc/config/riscv/riscv-ftypes.def
index 33620c57ca06..052acf0159d3 100644
--- a/gcc/config/riscv/riscv-ftypes.def
+++ b/gcc/config/riscv/riscv-ftypes.def
@@ -41,3 +41,4 @@ DEF_RISCV_FTYPE (2, (UDI, USI, USI))
 DEF_RISCV_FTYPE (2, (UDI, UDI, USI))
 DEF_RISCV_FTYPE (2, (UDI, UDI, UDI))
 DEF_RISCV_FTYPE (3, (USI, USI, USI, USI))
+DEF_RISCV_FTYPE_VAR (1, (VOID, VOID_PTR))
diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index e00b8ee3579d..eaa8b6a9f085 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -118,7 +118,9 @@
   UNSPECV_FLUSH
   UNSPECV_INVAL
   UNSPECV_ZERO
-  UNSPECV_PREI
+  UNSPECV_PREFETCH_I
+  UNSPECV_PREFETCH_R
+  UNSPECV_PREFETCH_W
 
   ;; Zihintpause unspec
   UNSPECV_PAUSE
@@ -3464,12 +3466,28 @@
 }
   [(set_attr "type" "cbo")])
 
-(define_insn "riscv_prefetchi_<mode>"
-  [(unspec_volatile:X [(match_operand:X 0 "address_operand" "r")
-              (match_operand:X 1 "imm5_operand" "i")]
-              UNSPECV_PREI)]
+(define_insn "riscv_prefetch_i_<mode>"
+  [(unspec_volatile:X [(match_operand:X 0 "register_operand" "r")
+		       (match_operand:X 1 "const_int_operand" "n")]
+		       UNSPECV_PREFETCH_I)]
   "TARGET_ZICBOP"
-  "prefetch.i\t%a0"
+  "prefetch.i\t%1(%0)"
+  [(set_attr "type" "cbo")])
+
+(define_insn "riscv_prefetch_r_<mode>"
+  [(unspec_volatile:X [(match_operand:X 0 "register_operand" "r")
+		       (match_operand:X 1 "const_int_operand" "n")]
+		       UNSPECV_PREFETCH_R)]
+  "TARGET_ZICBOP"
+  "prefetch.r\t%1(%0)"
+  [(set_attr "type" "cbo")])
+
+(define_insn "riscv_prefetch_w_<mode>"
+  [(unspec_volatile:X [(match_operand:X 0 "register_operand" "r")
+		       (match_operand:X 1 "const_int_operand" "n")]
+		       UNSPECV_PREFETCH_W)]
+  "TARGET_ZICBOP"
+  "prefetch.w\t%1(%0)"
   [(set_attr "type" "cbo")])
 
 (define_expand "extv<mode>"
diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-1.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-1.c
index c5d78c1763d3..f275d4633cce 100644
--- a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-1.c
+++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-1.c
@@ -1,23 +1,28 @@
-/* { dg-do compile target { { rv64-*-*}}} */
-/* { dg-options "-march=rv64gc_zicbop -mabi=lp64" } */
+/* { dg-do compile } */
+/* { dg-options "-march=rv32i_zicbop -mabi=ilp32" } */
 
 void foo (char *p)
 {
-  __builtin_prefetch (p, 0, 0);
-  __builtin_prefetch (p, 0, 1);
-  __builtin_prefetch (p, 0, 2);
-  __builtin_prefetch (p, 0, 3);
-  __builtin_prefetch (p, 1, 0);
-  __builtin_prefetch (p, 1, 1);
-  __builtin_prefetch (p, 1, 2);
-  __builtin_prefetch (p, 1, 3);
+  __builtin_riscv_prefetch_i (p);
+  __builtin_riscv_prefetch_i (p, 0);
+  __builtin_riscv_prefetch_i (p, 32);
+  __builtin_riscv_prefetch_i (p, -64);
+  __builtin_riscv_prefetch_r (p);
+  __builtin_riscv_prefetch_r (p, 0);
+  __builtin_riscv_prefetch_r (p, 32);
+  __builtin_riscv_prefetch_r (p, -64);
+  __builtin_riscv_prefetch_w (p);
+  __builtin_riscv_prefetch_w (p, 0);
+  __builtin_riscv_prefetch_w (p, 32);
+  __builtin_riscv_prefetch_w (p, -64);
 }
 
-int foo1()
-{
-  return __builtin_riscv_zicbop_cbo_prefetchi(1);
-}
-
-/* { dg-final { scan-assembler-times "prefetch.i" 1 } } */
-/* { dg-final { scan-assembler-times "prefetch.r" 4 } } */
-/* { dg-final { scan-assembler-times "prefetch.w" 4 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.i\t0\\("   2 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.i\t32\\("  1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.i\t-64\\(" 1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.r\t0\\("   2 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.r\t32\\("  1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.r\t-64\\(" 1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w\t0\\("   2 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w\t32\\("  1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w\t-64\\(" 1 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-2.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-2.c
index 6576365b39ca..17fff9e199da 100644
--- a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-2.c
+++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-2.c
@@ -1,23 +1,14 @@
-/* { dg-do compile target { { rv32-*-*}}} */
-/* { dg-options "-march=rv32gc_zicbop -mabi=ilp32" } */
+/* { dg-do compile } */
+/* { dg-options "-march=rv64i_zicbop -mabi=lp64" } */
 
-void foo (char *p)
-{
-  __builtin_prefetch (p, 0, 0);
-  __builtin_prefetch (p, 0, 1);
-  __builtin_prefetch (p, 0, 2);
-  __builtin_prefetch (p, 0, 3);
-  __builtin_prefetch (p, 1, 0);
-  __builtin_prefetch (p, 1, 1);
-  __builtin_prefetch (p, 1, 2);
-  __builtin_prefetch (p, 1, 3);
-}
+#include "cmo-zicbop-1.c"
 
-int foo1()
-{
-  return __builtin_riscv_zicbop_cbo_prefetchi(1);
-}
-
-/* { dg-final { scan-assembler-times "prefetch.i" 1 } } */
-/* { dg-final { scan-assembler-times "prefetch.r" 4 } } */
-/* { dg-final { scan-assembler-times "prefetch.w" 4 } } */ 
+/* { dg-final { scan-assembler-times "prefetch\\.i\t0\\("   2 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.i\t32\\("  1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.i\t-64\\(" 1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.r\t0\\("   2 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.r\t32\\("  1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.r\t-64\\(" 1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w\t0\\("   2 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w\t32\\("  1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w\t-64\\(" 1 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-3.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-3.c
new file mode 100644
index 000000000000..89bf6c57a390
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-3.c
@@ -0,0 +1,29 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv32i_zicbop -mabi=ilp32" } */
+
+void foo (void)
+{
+  /* Use NULL (0) instead of a pointer.  */
+  __builtin_riscv_prefetch_i (0);
+  __builtin_riscv_prefetch_i (0, 0);
+  __builtin_riscv_prefetch_i (0, 32);
+  __builtin_riscv_prefetch_i (0, -64);
+  __builtin_riscv_prefetch_r (0);
+  __builtin_riscv_prefetch_r (0, 0);
+  __builtin_riscv_prefetch_r (0, 32);
+  __builtin_riscv_prefetch_r (0, -64);
+  __builtin_riscv_prefetch_w (0);
+  __builtin_riscv_prefetch_w (0, 0);
+  __builtin_riscv_prefetch_w (0, 32);
+  __builtin_riscv_prefetch_w (0, -64);
+}
+
+/* { dg-final { scan-assembler-times "prefetch\\.i\t0\\("   2 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.i\t32\\("  1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.i\t-64\\(" 1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.r\t0\\("   2 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.r\t32\\("  1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.r\t-64\\(" 1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w\t0\\("   2 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w\t32\\("  1 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w\t-64\\(" 1 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-4.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-4.c
new file mode 100644
index 000000000000..372fdfa3d868
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-4.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv32i_zicbop -mabi=ilp32" } */
+/* { dg-skip-if "" { *-*-* } { "-flto"} } */
+
+int foo (char* p, int offset)
+{
+  __builtin_riscv_prefetch_i();
+  __builtin_riscv_prefetch_r();
+  __builtin_riscv_prefetch_w();
+}
+
+/* { dg-error "too few arguments to function '__builtin_riscv_prefetch_i'" "" { target *-*-* } 7 } */
+/* { dg-error "too few arguments to function '__builtin_riscv_prefetch_r'" "" { target *-*-* } 8 } */
+/* { dg-error "too few arguments to function '__builtin_riscv_prefetch_w'" "" { target *-*-* } 9 } */
diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-5.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-5.c
new file mode 100644
index 000000000000..7dbfe49b24fd
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-5.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv32i_zicbop -mabi=ilp32" } */
+/* { dg-skip-if "" { *-*-* } { "-flto"} } */
+
+int foo (char* p, int offset)
+{
+  __builtin_riscv_prefetch_i(p, offset);
+  __builtin_riscv_prefetch_r(p, offset);
+  __builtin_riscv_prefetch_w(p, offset);
+}
+
+/* { dg-error "second argument to '__builtin_riscv_prefetch_i' must be a constant" "not compile-time constant" { target *-*-* } 7 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_r' must be a constant" "not compile-time constant" { target *-*-* } 8 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_w' must be a constant" "not compile-time constant" { target *-*-* } 9 } */
diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-6.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-6.c
new file mode 100644
index 000000000000..b8deb2d6c807
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-6.c
@@ -0,0 +1,38 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv32i_zicbop -mabi=ilp32" } */
+/* { dg-skip-if "" { *-*-* } { "-flto"} } */
+
+int foo (char* p)
+{
+  __builtin_riscv_prefetch_i(p, 1);
+  __builtin_riscv_prefetch_r(p, 1);
+  __builtin_riscv_prefetch_w(p, 1);
+  __builtin_riscv_prefetch_i(p, 16);
+  __builtin_riscv_prefetch_r(p, 16);
+  __builtin_riscv_prefetch_w(p, 16);
+  __builtin_riscv_prefetch_i(p, -2080);
+  __builtin_riscv_prefetch_r(p, -2080);
+  __builtin_riscv_prefetch_w(p, -2080);
+  __builtin_riscv_prefetch_i(p, +2048);
+  __builtin_riscv_prefetch_r(p, +2048);
+  __builtin_riscv_prefetch_w(p, +2048);
+  __builtin_riscv_prefetch_i(p, 0x800000000000000llu);
+  __builtin_riscv_prefetch_r(p, 0x800000000000000llu);
+  __builtin_riscv_prefetch_w(p, 0x800000000000000llu);
+}
+
+/* { dg-error "second argument to '__builtin_riscv_prefetch_i' must be a valid offset for the prefetch instruction" "unaligned offset 1" { target *-*-* }  7 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_r' must be a valid offset for the prefetch instruction" "unaligned offset 1" { target *-*-* }  8 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_w' must be a valid offset for the prefetch instruction" "unaligned offset 1" { target *-*-* }  9 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_i' must be a valid offset for the prefetch instruction" "unaligned offset 2" { target *-*-* } 10 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_r' must be a valid offset for the prefetch instruction" "unaligned offset 2" { target *-*-* } 11 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_w' must be a valid offset for the prefetch instruction" "unaligned offset 2" { target *-*-* } 12 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_i' must be a valid offset for the prefetch instruction" "out of range (negative)" { target *-*-* } 13 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_r' must be a valid offset for the prefetch instruction" "out of range (negative)" { target *-*-* } 14 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_w' must be a valid offset for the prefetch instruction" "out of range (negative)" { target *-*-* } 15 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_i' must be a valid offset for the prefetch instruction" "out of range (positive 1)" { target *-*-* } 16 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_r' must be a valid offset for the prefetch instruction" "out of range (positive 1)" { target *-*-* } 17 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_w' must be a valid offset for the prefetch instruction" "out of range (positive 1)" { target *-*-* } 18 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_i' must be a valid offset for the prefetch instruction" "out of range (positive 2)" { target *-*-* } 19 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_r' must be a valid offset for the prefetch instruction" "out of range (positive 2)" { target *-*-* } 20 } */
+/* { dg-error "second argument to '__builtin_riscv_prefetch_w' must be a valid offset for the prefetch instruction" "out of range (positive 2)" { target *-*-* } 21 } */
diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-1.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-1.c
new file mode 100644
index 000000000000..14b89b8ddf35
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-1.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv32i_zicbop -mabi=ilp32" } */
+
+void foo (char *p)
+{
+  __builtin_prefetch (p, 0, 0);
+  __builtin_prefetch (p, 0, 1);
+  __builtin_prefetch (p, 0, 2);
+  __builtin_prefetch (p, 0, 3);
+  __builtin_prefetch (p, 1, 0);
+  __builtin_prefetch (p, 1, 1);
+  __builtin_prefetch (p, 1, 2);
+  __builtin_prefetch (p, 1, 3);
+}
+
+/* { dg-final { scan-assembler-times "prefetch\\.r" 4 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w" 4 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-2.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-2.c
new file mode 100644
index 000000000000..2fa38dccfcb0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-2.c
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64i_zicbop -mabi=lp64" } */
+
+#include "cmo-zicbop-by-common-1.c"
+
+/* { dg-final { scan-assembler-times "prefetch\\.r" 4 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w" 4 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-3.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-3.c
new file mode 100644
index 000000000000..37505d3d13c3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-3.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv32i_zicbop -mabi=ilp32" } */
+
+void foo (char *p)
+{
+  /* Second argument defaults to zero (read).  */
+  __builtin_prefetch (p);
+  __builtin_prefetch (p, 0);
+  __builtin_prefetch (p, 1);
+}
+
+/* { dg-final { scan-assembler-times "prefetch\\.r" 2 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w" 1 } } */
-- 
2.42.0

[PATCH 2/2] RISC-V: Fix ICE by expansion and register coercion

[Tsukasa OI]

From: Tsukasa OI <research_trasio@irq.a4lg.com>

A "prefetch" instruction on RISC-V GCC emits a machine hint instruction
directly when the 'Zicbop' extension is enabled but it could cause an ICE
when the address argument of __builtin_prefetch is a integral constant
(such like 0 [NULL] or some other [but possibly not all] fixed addresses).

It fixes the problem by changing "prefetch" from a native instruction to
an expansion and coercing the address to a register there.

gcc/ChangeLog:

	* config/riscv/riscv.md (prefetch): Expand to a native prefetch
	instruction instead of emitting a machine instruction directly.
	Coerce the address argument into a register.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/cmo-zicbop-by-common-ice-1.c: New ICE test.
	* gcc.target/riscv/cmo-zicbop-by-common-ice-2.c: Ditto.
---
 gcc/config/riscv/riscv.md                     | 43 ++++++++++++-------
 .../riscv/cmo-zicbop-by-common-ice-1.c        | 13 ++++++
 .../riscv/cmo-zicbop-by-common-ice-2.c        |  7 +++
 3 files changed, 48 insertions(+), 15 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-ice-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-ice-2.c

diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index eaa8b6a9f085..12e78b60980e 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -3451,21 +3451,6 @@
   [(set_attr "type" "cbo")]
 )
 
-(define_insn "prefetch"
-  [(prefetch (match_operand 0 "address_operand" "r")
-             (match_operand 1 "imm5_operand" "i")
-             (match_operand 2 "const_int_operand" "n"))]
-  "TARGET_ZICBOP"
-{
-  switch (INTVAL (operands[1]))
-  {
-    case 0: return "prefetch.r\t%a0";
-    case 1: return "prefetch.w\t%a0";
-    default: gcc_unreachable ();
-  }
-}
-  [(set_attr "type" "cbo")])
-
 (define_insn "riscv_prefetch_i_<mode>"
   [(unspec_volatile:X [(match_operand:X 0 "register_operand" "r")
 		       (match_operand:X 1 "const_int_operand" "n")]
@@ -3490,6 +3475,34 @@
   "prefetch.w\t%1(%0)"
   [(set_attr "type" "cbo")])
 
+(define_expand "prefetch"
+  [(prefetch (match_operand 0 "address_operand" "")
+	     (match_operand 1 "const_int_operand" "")
+	     (match_operand 2 "const_int_operand" ""))]
+  "TARGET_ZICBOP"
+{
+  operands[0] = force_reg (Pmode, operands[0]);
+  switch (INTVAL (operands[1]))
+    {
+    case 0:
+      if (TARGET_64BIT)
+	emit_insn (gen_riscv_prefetch_r_di (operands[0], const0_rtx));
+      else
+	emit_insn (gen_riscv_prefetch_r_si (operands[0], const0_rtx));
+      break;
+    case 1:
+      if (TARGET_64BIT)
+	emit_insn (gen_riscv_prefetch_w_di (operands[0], const0_rtx));
+      else
+	emit_insn (gen_riscv_prefetch_w_si (operands[0], const0_rtx));
+      break;
+    default:
+      gcc_unreachable ();
+    }
+  DONE;
+}
+  [(set_attr "type" "cbo")])
+
 (define_expand "extv<mode>"
   [(set (match_operand:GPR 0 "register_operand" "=r")
 	(sign_extract:GPR (match_operand:GPR 1 "register_operand" "r")
diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-ice-1.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-ice-1.c
new file mode 100644
index 000000000000..47e83f29cc5c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-ice-1.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv32i_zicbop -mabi=ilp32" } */
+
+void foo (void)
+{
+  /* Second argument defaults to zero (read).  */
+  __builtin_prefetch (0);
+  __builtin_prefetch (0, 0);
+  __builtin_prefetch (0, 1);
+}
+
+/* { dg-final { scan-assembler-times "prefetch\\.r" 2 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w" 1 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-ice-2.c b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-ice-2.c
new file mode 100644
index 000000000000..a245b8163c1f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/cmo-zicbop-by-common-ice-2.c
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64i_zicbop -mabi=lp64" } */
+
+#include "cmo-zicbop-by-common-ice-1.c"
+
+/* { dg-final { scan-assembler-times "prefetch\\.r" 2 } } */
+/* { dg-final { scan-assembler-times "prefetch\\.w" 1 } } */
-- 
2.42.0

Re: [PATCH 0/2] RISC-V: Define not broken prefetch builtins

[Jeff Law]

On 9/22/23 01:11, Tsukasa OI wrote:
> Hello,
> 
> As I explained earlier:
> <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626916.html>,
> the builtin function for RISC-V "__builtin_riscv_zicbop_cbo_prefetchi" is
> completely broken.  Instead, this patch set (in PATCH 1/2) creates three
> new, working builtin intrinsics.
> 
> void __builtin_riscv_prefetch_i(void *addr, [intptr_t offset,] ...);
> void __builtin_riscv_prefetch_r(void *addr, [intptr_t offset,] ...);
> void __builtin_riscv_prefetch_w(void *addr, [intptr_t offset,] ...);
> 
> 
> For consistency with "prefetch.i" and the reason I describe later (which
> requires native instructions for "prefetch.r" and "prefetch.w"), I decided
> to make builtin functions for "prefetch.[rw]" as well.
> 
> Optional second argument (named "offset" here) defaults to zero and must be
> a compile-time integral constant.  Also, it must be a valid offset for a
> "prefetch.[irw]" HINT instruction (x % 32 == 0 && x >= -2048 && x < 2048).
> 
> They are defined if the 'Zicbop' extension is supported and expands to:
> 
>> prefetch.i offset(addr_reg)  ; __builtin_riscv_prefetch_i
>> prefetch.r offset(addr_reg)  ; __builtin_riscv_prefetch_r
>> prefetch.w offset(addr_reg)  ; __builtin_riscv_prefetch_w
> 
> 
> The hardest part of this patch set was to support builtin function with
> variable argument (making "offset" optional).  It required:
> 
> 1.  Support for variable argument function prototype for RISC-V builtins
>      (corresponding "..." on C-based languages)
> 2.  Support for (non-vector) RISC-V builtins with custom expansion
>      (on RVV intrinsics, custom expansion is already implemented)
> 
> 
> ... and PATCH 2/2 fixes an ICE while I'm investigating regular prefetch
> builtin (__builtin_prefetch).  If the 'Zicbop' extension is enabled,
> __builtin_prefetch with the first argument NULL or (not all but) some
> fixed addresses (like ((void*)0x20)) can cause an ICE.  This is because
> the "r" constraint is not checked and a constant can be a first argument
> of target-specific "prefetch" RTL instruction.
> 
> PATCH 2/2 fixes this issue by:
> 
> 1.  Making "prefetch" not an instruction but instead an expansion
>      (this is not rare; e.g. on i386) and
> 2.  Coercing the address argument into a register in the expansion
> 
> It requires separate instructions for "prefetch.[rw]" and I decided to make
> those prefetch instructions very similar to "prefetch.i".  That's one of the
> reasons I created builtins corresponding those.
What I still don't understand is why we're dealing with a decomposed 
address in the builtin, define_expand and/or define_insn.

Have the builtin accept an address, any address.  Then use force_reg to 
force the address into a register in the expander.  My understanding is 
register indirect is always valid.

Create an operand predicate that accepts reg and reg+d for the limited 
displacements allowed.  Use that for the address operand in the 
associated define_insn.


It seems like you're making this more complex than it needs to be.  Or 
I'm missing something critically important.

jeff

Re: [PATCH 0/2] RISC-V: Define not broken prefetch builtins

[Tsukasa OI]

On 2023/09/27 6:38, Jeff Law wrote:
> 
> 
> On 9/22/23 01:11, Tsukasa OI wrote:
>> Hello,
>>
>> As I explained earlier:
>> <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626916.html>,
>> the builtin function for RISC-V "__builtin_riscv_zicbop_cbo_prefetchi" is
>> completely broken.  Instead, this patch set (in PATCH 1/2) creates three
>> new, working builtin intrinsics.
>>
>> void __builtin_riscv_prefetch_i(void *addr, [intptr_t offset,] ...);
>> void __builtin_riscv_prefetch_r(void *addr, [intptr_t offset,] ...);
>> void __builtin_riscv_prefetch_w(void *addr, [intptr_t offset,] ...);
>>
>>
>> For consistency with "prefetch.i" and the reason I describe later (which
>> requires native instructions for "prefetch.r" and "prefetch.w"), I
>> decided
>> to make builtin functions for "prefetch.[rw]" as well.
>>
>> Optional second argument (named "offset" here) defaults to zero and
>> must be
>> a compile-time integral constant.  Also, it must be a valid offset for a
>> "prefetch.[irw]" HINT instruction (x % 32 == 0 && x >= -2048 && x <
>> 2048).
>>
>> They are defined if the 'Zicbop' extension is supported and expands to:
>>
>>> prefetch.i offset(addr_reg)  ; __builtin_riscv_prefetch_i
>>> prefetch.r offset(addr_reg)  ; __builtin_riscv_prefetch_r
>>> prefetch.w offset(addr_reg)  ; __builtin_riscv_prefetch_w
>>
>>
>> The hardest part of this patch set was to support builtin function with
>> variable argument (making "offset" optional).  It required:
>>
>> 1.  Support for variable argument function prototype for RISC-V builtins
>>      (corresponding "..." on C-based languages)
>> 2.  Support for (non-vector) RISC-V builtins with custom expansion
>>      (on RVV intrinsics, custom expansion is already implemented)
>>
>>
>> ... and PATCH 2/2 fixes an ICE while I'm investigating regular prefetch
>> builtin (__builtin_prefetch).  If the 'Zicbop' extension is enabled,
>> __builtin_prefetch with the first argument NULL or (not all but) some
>> fixed addresses (like ((void*)0x20)) can cause an ICE.  This is because
>> the "r" constraint is not checked and a constant can be a first argument
>> of target-specific "prefetch" RTL instruction.
>>
>> PATCH 2/2 fixes this issue by:
>>
>> 1.  Making "prefetch" not an instruction but instead an expansion
>>      (this is not rare; e.g. on i386) and
>> 2.  Coercing the address argument into a register in the expansion
>>
>> It requires separate instructions for "prefetch.[rw]" and I decided to
>> make
>> those prefetch instructions very similar to "prefetch.i".  That's one
>> of the
>> reasons I created builtins corresponding those.
> What I still don't understand is why we're dealing with a decomposed
> address in the builtin, define_expand and/or define_insn.

Sorry, I misunderstood your intent (quite badly) possibly because I was
not familiar with the concept of "predicates" in GCC.

On 2023/08/29 6:20, Jeff Law wrote:
> What I would suggest is making a new predicate that accepts either a 
> register or a register+offset where the offset fits in a signed 12 bit 
> immediate.  Use that for operand 0's predicate and I think this will 
> "just work" and cover all the cases supported by the prefetch.i instruction.

I misunderstood that as "just" adding the offset field to the
instructions and that's the reason I veered off the path so much.  So
instead, I'll answer your original question.

register+offset seems a problem for prefetch instructions because signed
12 bit immediate values need to be also a multiple of 32.  There's no
proper relocation type for this kind and I considered we have "very"
limited cases where making such predicate (as you suggested) will
*efficiently* work.

My opinion is, if we need very fine-grained control with prefetch
instructions, we'd better to use inline assembly.

I'll continue testing the possibilities of register+offset predicate
(including whether it works efficiently) and I'll temporarily withdraw
new built-in functions to focus on major issues before GCC 14:

1.  Remove completely broken __builtin_riscv_zicbop_prefetch_i and
2.  Fix an ICE when __builtin_prefetch is used with some constants.

I'll submit minimized patches only to fix those issues.  They will not
contain "register+offset" you suggested because of the difficulties
above but should be sufficient to fix imminent issues.

Thanks,
Tsukasa

> 
> Have the builtin accept an address, any address.  Then use force_reg to
> force the address into a register in the expander.  My understanding is
> register indirect is always valid.
> 
> Create an operand predicate that accepts reg and reg+d for the limited
> displacements allowed.  Use that for the address operand in the
> associated define_insn.
> 
> 
> It seems like you're making this more complex than it needs to be.  Or
> I'm missing something critically important.
> 
> jeff
> 

Re: [PATCH 0/2] RISC-V: Define not broken prefetch builtins

[Jeff Law]

On 10/22/23 21:55, Tsukasa OI wrote:

>> What I still don't understand is why we're dealing with a decomposed
>> address in the builtin, define_expand and/or define_insn.

> 
> Sorry, I misunderstood your intent (quite badly) possibly because I was
> not familiar with the concept of "predicates" in GCC.
OK.  So you might want to read the machine description part of the GCC 
manual.  It describes operand predicates, operand constraints, insn 
conditions, the difference between define_insn vs define_expand and much 
more.


> 
> On 2023/08/29 6:20, Jeff Law wrote:
>> What I would suggest is making a new predicate that accepts either a
>> register or a register+offset where the offset fits in a signed 12 bit
>> immediate.  Use that for operand 0's predicate and I think this will
>> "just work" and cover all the cases supported by the prefetch.i instruction.
> 
> I misunderstood that as "just" adding the offset field to the
> instructions and that's the reason I veered off the path so much.  So
> instead, I'll answer your original question.
> 
> register+offset seems a problem for prefetch instructions because signed
> 12 bit immediate values need to be also a multiple of 32.  There's no
> proper relocation type for this kind and I considered we have "very"
> limited cases where making such predicate (as you suggested) will
> *efficiently* work.
> 
> My opinion is, if we need very fine-grained control with prefetch
> instructions, we'd better to use inline assembly.
> 
> I'll continue testing the possibilities of register+offset predicate
> (including whether it works efficiently) and I'll temporarily withdraw
> new built-in functions to focus on major issues before GCC 14:
> 
> 1.  Remove completely broken __builtin_riscv_zicbop_prefetch_i and
> 2.  Fix an ICE when __builtin_prefetch is used with some constants.
> 
> I'll submit minimized patches only to fix those issues.  They will not
> contain "register+offset" you suggested because of the difficulties
> above but should be sufficient to fix imminent issues.
We should be able to describe this need quite easily.

Each operand has a predicate which the compiler tests to see if a 
particular RTL expression matches.  Some are very generic like 
"register_operand".  Others are target specific.  If you look in 
predicates.md you'll see a list of the predicates already defined for 
risc-v.  I'm pretty sure none of them will work for this case, but we 
can add a new one easily.

The operand in question is going to be a MEM with restrictions on its 
addressing mode.  Either REG or REG + aligned offset.

(define_predicate "prefetch_memory_operand"
   (match_code "mem")
{
   op = XEXP (op, 0);
   return (REG_P (op)
           || (GET_CODE (op) == PLUS
               && REG_P (XEXP (op, 0))
               && CONST_INT_P (XEXP (op, 1))
               && (INTVAL (XEXP (op, 1)) % 32) == 0);
}

[ Note that we did not declare "op".  It's provided by the generator and 
corresponds to the operand we're testing. ]

So you're going to want a define_expand for the basic prefetch

(define_expand "riscv_prefetch_r_<mode>"
   [(unspec_volatile:X [(match_operand:X 0 "memory_operand")]
                        UNSPEC_PREFETCH_R)]
   "TARGET_ZICBOP"
{
   if (!prefetch_memory_operand (Pmode, operands[0])
     XEXP (operands[0], 0) = force_reg (Pmode, XEXP (operands[0], 0);
}

The thing to know about a define_expand is that it's sole purpose is for 
RTL generation purposes.   We can use it as a place to adjust operands 
(as is done in this case), or emit additional RTL such as we do for 
SImode max on rv64 where we have to extend the incoming operands.

In our case we see if the memory address matches 
prefetch_memory_operand, and if not it'll force that address into a new 
register to create a (mem (reg)) object.



(define_insn "*riscv_prefetch_r_<mode>"
   [(unspec_volatile:X [(match_operand:X 0 "prefetch_memory_operand")]
                        UNSPEC_PREFETCH_R)]
   "TARGET_ZICBOP"
   "prefetch.r\t%0"
   [(set_attr "type" "cbo")])

The define_insn construct maps an RTL template to assembly code with 
provisions for testing operands and such.

Anyway, hopefully that makes things clearer.


Jeff