diff mbox series

Allow target to chose address-space for artificial rodata lookup tables.

Message ID fbc5ff24-90e0-4c00-b479-a712e3ee23c5@gjlay.de
State New
Headers show
Series Allow target to chose address-space for artificial rodata lookup tables. | expand

Commit Message

Georg-Johann Lay Dec. 9, 2024, 2:13 p.m. UTC 
This patch adds a new target hook that allows to chose
a non-generic named address-space for compiler generated
lookup tables.

The purpose is that there are cases (on avr namely) where
the generic address space is sub-optimal because it must
put .rodata in RAM.  With this hook it is possible to
chose an address space that's better suited, like the
__flash address space that allocates to .progmem.data which
resides in flash.

The patch passes without regressions on avr.

On x86_64, it bootstraps and tests without regressions.

Ok for trunk?

Johann

p.s.  The feature has been discussed in the lists before,
and in all discussions I failed in getting across why a
different address space is needed.  Some background:

1) On AVR, you cannot just put data in a different section
without also using different instructions to access it.
In general a different section also requires different
address registers and different addressing modes and
different instructions.

2) It is *not* possible to do this during linker relaxation.
You cannot just change register allocation and address registers
in the linker.  You cannot just replace a 16-bit register like
X or Y by a 24-bit address that lives in Z (lower 16 bits) and
in some SFR (upper 8 bits).

3) You cannot just put all static storage read-only data into
a different address space.  For example, it is perfectly fine
for a C/C++ code to define a variable in static storage and
access it in assembly code.  The assembly code must know the
address space of the symbol, or otherwise the code is wrong.

4) From 3) it follows that you can only change the address space
of an object when it is hidden from the user, i.e. the compiler
is building the object and has control over all accesses, and
there's no way the user can get a reference to the object.

To date, there are only 2 lookup tables generated by GCC that
fit these criteria:

A) CSWTCH tables from tree-switch-conversion.cc.

B) crc_table_for_* tables from gimple-crc-optimization.cc.

Though B) may increase the code size by quite a lot.  For example,
size of gcc.dg/torture/crc-2.c will increase by more than 1500%
(and even more when a 24-bit address-space is required).  The
CRC optimizations uses some builtin magic, so it's unclear where
and how to introduce a different address space.

--

Allow target to chose address-space for artificial rodata.

gcc/
	* coretypes.h (enum artificial_rodata): New enum type.
	* doc/tm.texi: Rebuild.
	* doc/tm.texi.in (TARGET_ADDR_SPACE_FOR_ARTIFICIAL_RODATA):
	New hook.
	* target.def (addr_sapce.for_artificial_rodata): New DEFHOOK.
	* targhooks.cc (default_addr_space_convert): New function.
	* targhooks.h (default_addr_space_convert): New prototype.
	* tree-switch-conversion.cc (build_one_array) <value_type>:
	Set type_quals address-space according to
	targetm.addr_space.for_artificial_rodata().

	* config/avr/avr.cc (avr_rodata_in_flash_p): Move up.
	(TARGET_ADDR_SPACE_FOR_ARTIFICIAL_RODATA): Define to...
	(avr_addr_space_for_artificial_rodata): ...this new function.

Comments

Georg-Johann Lay Dec. 12, 2024, 11:17 a.m. UTC | #1 
For the avr.cc part, the __flashx named address space has been approved,
which means that the natural choice in the target hook would be
ADDR_SPACE_FLASHX instead of ADDR_SPACE_MEMX:

> +/* Implement `TARGET_ADDR_SPACE_FOR_ARTIFICIAL_RODATA'.  */
> +
> +static addr_space_t
> +avr_addr_space_for_artificial_rodata (tree /*type*/,
> +				      artificial_rodata /*kind*/)
> +{
> +  return avr_rodata_in_flash_p ()
> +    ? ADDR_SPACE_GENERIC
> +    : avropt_n_flash > 1 ? ADDR_SPACE_MEMX : ADDR_SPACE_FLASH;
> +}
> +

Johann

Am 09.12.24 um 15:13 schrieb Georg-Johann Lay:
> This patch adds a new target hook that allows to chose
> a non-generic named address-space for compiler generated
> lookup tables.
> 
> The purpose is that there are cases (on avr namely) where
> the generic address space is sub-optimal because it must
> put .rodata in RAM.  With this hook it is possible to
> chose an address space that's better suited, like the
> __flash address space that allocates to .progmem.data which
> resides in flash.
> 
> The patch passes without regressions on avr.
> 
> On x86_64, it bootstraps and tests without regressions.
> 
> Ok for trunk?
> 
> Johann
> 
> p.s.  The feature has been discussed in the lists before,
> and in all discussions I failed in getting across why a
> different address space is needed.  Some background:
> 
> 1) On AVR, you cannot just put data in a different section
> without also using different instructions to access it.
> In general a different section also requires different
> address registers and different addressing modes and
> different instructions.
> 
> 2) It is *not* possible to do this during linker relaxation.
> You cannot just change register allocation and address registers
> in the linker.  You cannot just replace a 16-bit register like
> X or Y by a 24-bit address that lives in Z (lower 16 bits) and
> in some SFR (upper 8 bits).
> 
> 3) You cannot just put all static storage read-only data into
> a different address space.  For example, it is perfectly fine
> for a C/C++ code to define a variable in static storage and
> access it in assembly code.  The assembly code must know the
> address space of the symbol, or otherwise the code is wrong.
> 
> 4) From 3) it follows that you can only change the address space
> of an object when it is hidden from the user, i.e. the compiler
> is building the object and has control over all accesses, and
> there's no way the user can get a reference to the object.
> 
> To date, there are only 2 lookup tables generated by GCC that
> fit these criteria:
> 
> A) CSWTCH tables from tree-switch-conversion.cc.
> 
> B) crc_table_for_* tables from gimple-crc-optimization.cc.
> 
> Though B) may increase the code size by quite a lot.  For example,
> size of gcc.dg/torture/crc-2.c will increase by more than 1500%
> (and even more when a 24-bit address-space is required).  The
> CRC optimizations uses some builtin magic, so it's unclear where
> and how to introduce a different address space.
> 
> -- 
> 
> Allow target to chose address-space for artificial rodata.
> 
> gcc/
>      * coretypes.h (enum artificial_rodata): New enum type.
>      * doc/tm.texi: Rebuild.
>      * doc/tm.texi.in (TARGET_ADDR_SPACE_FOR_ARTIFICIAL_RODATA):
>      New hook.
>      * target.def (addr_sapce.for_artificial_rodata): New DEFHOOK.
>      * targhooks.cc (default_addr_space_convert): New function.
>      * targhooks.h (default_addr_space_convert): New prototype.
>      * tree-switch-conversion.cc (build_one_array) <value_type>:
>      Set type_quals address-space according to
>      targetm.addr_space.for_artificial_rodata().
> 
>      * config/avr/avr.cc (avr_rodata_in_flash_p): Move up.
>      (TARGET_ADDR_SPACE_FOR_ARTIFICIAL_RODATA): Define to...
>      (avr_addr_space_for_artificial_rodata): ...this new function.
diff mbox series

Patch

diff --git a/gcc/config/avr/avr.cc b/gcc/config/avr/avr.cc
index 7c7736781c8..7dc3eb2016a 100644
--- a/gcc/config/avr/avr.cc
+++ b/gcc/config/avr/avr.cc
@@ -119,6 +119,25 @@  const avr_addrspace_t avr_addrspace[ADDR_SPACE_COUNT] =
 };
 
 
+#ifdef HAVE_LD_AVR_AVRXMEGA2_FLMAP
+static const bool have_avrxmega2_flmap = true;
+#else
+static const bool have_avrxmega2_flmap = false;
+#endif
+
+#ifdef HAVE_LD_AVR_AVRXMEGA4_FLMAP
+static const bool have_avrxmega4_flmap = true;
+#else
+static const bool have_avrxmega4_flmap = false;
+#endif
+
+#ifdef HAVE_LD_AVR_AVRXMEGA3_RODATA_IN_FLASH
+static const bool have_avrxmega3_rodata_in_flash = true;
+#else
+static const bool have_avrxmega3_rodata_in_flash = false;
+#endif
+
+
 /* Holding RAM addresses of some SFRs used by the compiler and that
    are unique over all devices in an architecture like 'avr4'.  */
 
@@ -254,6 +273,31 @@  avr_tolower (char *lo, const char *up)
 }
 
 
+static bool
+avr_rodata_in_flash_p ()
+{
+  switch (avr_arch_index)
+    {
+    default:
+      break;
+
+    case ARCH_AVRTINY:
+      return true;
+
+    case ARCH_AVRXMEGA3:
+      return have_avrxmega3_rodata_in_flash;
+
+    case ARCH_AVRXMEGA2:
+      return avropt_flmap && have_avrxmega2_flmap && avropt_rodata_in_ram != 1;
+
+    case ARCH_AVRXMEGA4:
+      return avropt_flmap && have_avrxmega4_flmap && avropt_rodata_in_ram != 1;
+    }
+
+  return false;
+}
+
+
 /* Return chunk of mode MODE of X as an rtx.  N specifies the subreg
    byte at which the chunk starts.  N must be an integral multiple
    of the mode size.  */
@@ -11123,6 +11167,18 @@  avr_addr_space_diagnose_usage (addr_space_t as, location_t loc)
 }
 
 
+/* Implement `TARGET_ADDR_SPACE_FOR_ARTIFICIAL_RODATA'.  */
+
+static addr_space_t
+avr_addr_space_for_artificial_rodata (tree /*type*/,
+				      artificial_rodata /*kind*/)
+{
+  return avr_rodata_in_flash_p ()
+    ? ADDR_SPACE_GENERIC
+    : avropt_n_flash > 1 ? ADDR_SPACE_MEMX : ADDR_SPACE_FLASH;
+}
+
+
 /* Implement `TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID'.  Zero is a valid
    address in all address spaces. Even in ADDR_SPACE_FLASH1 etc..,
    a zero address is valid and means 0x<RAMPZ val>0000, where RAMPZ is
@@ -11396,49 +11452,6 @@  avr_insert_attributes (tree node, tree *attributes)
     }
 }
 
-#ifdef HAVE_LD_AVR_AVRXMEGA2_FLMAP
-static const bool have_avrxmega2_flmap = true;
-#else
-static const bool have_avrxmega2_flmap = false;
-#endif
-
-#ifdef HAVE_LD_AVR_AVRXMEGA4_FLMAP
-static const bool have_avrxmega4_flmap = true;
-#else
-static const bool have_avrxmega4_flmap = false;
-#endif
-
-#ifdef HAVE_LD_AVR_AVRXMEGA3_RODATA_IN_FLASH
-static const bool have_avrxmega3_rodata_in_flash = true;
-#else
-static const bool have_avrxmega3_rodata_in_flash = false;
-#endif
-
-
-static bool
-avr_rodata_in_flash_p ()
-{
-  switch (avr_arch_index)
-    {
-    default:
-      break;
-
-    case ARCH_AVRTINY:
-      return true;
-
-    case ARCH_AVRXMEGA3:
-      return have_avrxmega3_rodata_in_flash;
-
-    case ARCH_AVRXMEGA2:
-      return avropt_flmap && have_avrxmega2_flmap && avropt_rodata_in_ram != 1;
-
-    case ARCH_AVRXMEGA4:
-      return avropt_flmap && have_avrxmega4_flmap && avropt_rodata_in_ram != 1;
-    }
-
-  return false;
-}
-
 
 /* Implement `ASM_OUTPUT_ALIGNED_DECL_LOCAL'.  */
 /* Implement `ASM_OUTPUT_ALIGNED_DECL_COMMON'.  */
@@ -16013,6 +16026,10 @@  avr_use_lra_p ()
 #undef  TARGET_ADDR_SPACE_DIAGNOSE_USAGE
 #define TARGET_ADDR_SPACE_DIAGNOSE_USAGE avr_addr_space_diagnose_usage
 
+#undef  TARGET_ADDR_SPACE_FOR_ARTIFICIAL_RODATA
+#define TARGET_ADDR_SPACE_FOR_ARTIFICIAL_RODATA \
+  avr_addr_space_for_artificial_rodata
+
 #undef  TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
 #define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID avr_addr_space_zero_address_valid
 
diff --git a/gcc/coretypes.h b/gcc/coretypes.h
index e8ccba31634..ea450204e38 100644
--- a/gcc/coretypes.h
+++ b/gcc/coretypes.h
@@ -340,6 +340,19 @@  enum warn_strict_overflow_code
   WARN_STRICT_OVERFLOW_MAGNITUDE = 5
 };
 
+/* Kind of artificial, compiler-generated lookup table.  Type of the
+   second argument of TARGET_ADDR_SPACE_FOR_ARTIFICIAL_RODATA resp.
+   targetm.addr_space.for_artificial_rodata.  */
+enum artificial_rodata
+{
+  /* Generated by tree-switch-conversion.cc: Lowered GIMPLE_SWITCH expressions
+     to something more efficient than a jump table.  */
+  ARTIFICIAL_RODATA_CSWITCH,
+
+  /* Generated by gimple-crc-optimization.cc:  CRC optimization.  */
+  ARTIFICIAL_RODATA_CRC
+};
+
 /* The type of an alias set.  Code currently assumes that variables of
    this type can take the values 0 (the alias set which aliases
    everything) and -1 (sometimes indicating that the alias set is
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index fd60c704d50..b7bd40a3f65 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -11399,6 +11399,31 @@  the address space as registered with @code{c_register_addr_space}.
 The default implementation does nothing.
 @end deftypefn
 
+@deftypefn {Target Hook} addr_space_t TARGET_ADDR_SPACE_FOR_ARTIFICIAL_RODATA (tree @var{type}, enum artificial_rodata @var{purpose})
+Define this hook to return a named address space to be used for
+@var{type}, usually the type of an artificial lookup-table that would
+reside in @code{.rodata} and in the generic address space.
+
+The hook can be used to put compiler-generated, artificial lookup tables in
+static storage into a non-generic address space when it is better suited
+than the generic address space.
+The compiler will generate all accesses to the respective data
+so that all associated accesses will also use the specified address space
+and pointer mode.
+
+@var{type} is the type of the lookup table. @var{purpose} specifies
+the purpose of the lookup table.  It is one of:
+@table @code
+@item ARTIFICIAL_RODATA_CSWITCH
+@file{tree-switch-conversion.cc} lowered a GIMPLE_SWITCH expressions
+to something more efficient than a jump table.
+@item ARTIFICIAL_RODATA_CRC
+@file{gimple-crc-optimization.cc} optimized a CRC computation by
+using a polynomial lookup table.
+@end table
+The default implementation of the hook returns @code{ADDR_SPACE_GENERIC}.
+@end deftypefn
+
 @node Misc
 @section Miscellaneous Parameters
 @cindex parameters, miscellaneous
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index f6657f9df1d..a2e01bd941e 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -7332,6 +7332,8 @@  c_register_addr_space ("__ea", ADDR_SPACE_EA);
 
 @hook TARGET_ADDR_SPACE_DIAGNOSE_USAGE
 
+@hook TARGET_ADDR_SPACE_FOR_ARTIFICIAL_RODATA
+
 @node Misc
 @section Miscellaneous Parameters
 @cindex parameters, miscellaneous
diff --git a/gcc/target.def b/gcc/target.def
index 768ea7bd04a..54a4cb20fad 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -3504,6 +3504,35 @@  The default implementation does nothing.",
  void, (addr_space_t as, location_t loc),
  default_addr_space_diagnose_usage)
 
+/* Function to patch the address space of some compiler-generated
+   read-only data.  Used for optimization purposes only.  */
+DEFHOOK
+(for_artificial_rodata,
+ "Define this hook to return a named address space to be used for\n\
+@var{type}, usually the type of an artificial lookup-table that would\n\
+reside in @code{.rodata} and in the generic address space.\n\
+\n\
+The hook can be used to put compiler-generated, artificial lookup tables in\n\
+static storage into a non-generic address space when it is better suited\n\
+than the generic address space.\n\
+The compiler will generate all accesses to the respective data\n\
+so that all associated accesses will also use the specified address space\n\
+and pointer mode.\n\
+\n\
+@var{type} is the type of the lookup table. @var{purpose} specifies\n\
+the purpose of the lookup table.  It is one of:\n\
+@table @code\n\
+@item ARTIFICIAL_RODATA_CSWITCH\n\
+@file{tree-switch-conversion.cc} lowered a GIMPLE_SWITCH expressions\n\
+to something more efficient than a jump table.\n\
+@item ARTIFICIAL_RODATA_CRC\n\
+@file{gimple-crc-optimization.cc} optimized a CRC computation by\n\
+using a polynomial lookup table.\n\
+@end table\n\
+The default implementation of the hook returns @code{ADDR_SPACE_GENERIC}.",
+ addr_space_t, (tree type, enum artificial_rodata purpose),
+ default_addr_space_for_artificial_rodata)
+
 HOOK_VECTOR_END (addr_space)
 
 #undef HOOK_PREFIX
diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc
index 8ea8d778003..bf6d2583585 100644
--- a/gcc/targhooks.cc
+++ b/gcc/targhooks.cc
@@ -1783,6 +1783,16 @@  default_addr_space_convert (rtx op ATTRIBUTE_UNUSED,
   gcc_unreachable ();
 }
 
+
+/* The default hook for TARGET_ADDR_SPACE_FOR_ARTIFICIAL_RODATA.  */
+
+addr_space_t
+default_addr_space_for_artificial_rodata (tree, artificial_rodata)
+{
+  return ADDR_SPACE_GENERIC;
+}
+
+
 /* The defualt implementation of TARGET_HARD_REGNO_NREGS.  */
 
 unsigned int
diff --git a/gcc/targhooks.h b/gcc/targhooks.h
index 7cf22038100..0920018d808 100644
--- a/gcc/targhooks.h
+++ b/gcc/targhooks.h
@@ -214,6 +214,8 @@  extern bool default_addr_space_subset_p (addr_space_t, addr_space_t);
 extern bool default_addr_space_zero_address_valid (addr_space_t);
 extern int default_addr_space_debug (addr_space_t);
 extern void default_addr_space_diagnose_usage (addr_space_t, location_t);
+extern addr_space_t default_addr_space_for_artificial_rodata (tree,
+							      artificial_rodata);
 extern rtx default_addr_space_convert (rtx, tree, tree);
 extern unsigned int default_case_values_threshold (void);
 extern bool default_have_conditional_execution (void);
diff --git a/gcc/tree-switch-conversion.cc b/gcc/tree-switch-conversion.cc
index 3436c2a8b98..46af42bcf18 100644
--- a/gcc/tree-switch-conversion.cc
+++ b/gcc/tree-switch-conversion.cc
@@ -1007,6 +1007,16 @@  switch_conversion::build_one_array (int num, tree arr_index_type,
       default_type = TREE_TYPE (m_default_values[num]);
       value_type = array_value_type (default_type, num);
       array_type = build_array_type (value_type, arr_index_type);
+      addr_space_t as
+	= targetm.addr_space.for_artificial_rodata (array_type,
+						    ARTIFICIAL_RODATA_CSWITCH);
+      if (!ADDR_SPACE_GENERIC_P (as))
+	{
+	  int quals = (TYPE_QUALS_NO_ADDR_SPACE (value_type)
+		       | ENCODE_QUAL_ADDR_SPACE (as));
+	  value_type = build_qualified_type (value_type, quals);
+	  array_type = build_array_type (value_type, arr_index_type);
+	}
       if (default_type != value_type)
 	{
 	  unsigned int i;