Re: [Ping] Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523]

2023-06-17 Thread Jeff Law via Gcc-patches 




On 6/16/23 04:17, SenthilKumar.Selvaraj--- via Gcc-patches wrote:

On Fri, 2023-06-02 at 12:32 +0530, Senthil Kumar Selvaraj wrote:

On Mon, 2023-05-22 at 14:05 +0200, Richard Biener wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you know the 
content is safe

On Fri, May 19, 2023 at 7:58 AM  wrote:

On 26/04/23, 5:51 PM, "Richard Biener" mailto:richard.guent...@gmail.com>> wrote:

On Wed, Apr 26, 2023 at 12:56 PM mailto:senthilkumar.selva...@microchip.com>> wrote:

On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches mailto:gcc-patches@gcc.gnu.org>> wrote:

On Wed, Apr 26, 2023 at 11:42 AM Richard Biener
mailto:richard.guent...@gmail.com>> wrote:

On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via
Gcc-patches mailto:gcc-patches@gcc.gnu.org>> wrote:

Hi,

This patch fixes PR 105523 by setting param_min_pagesize to 0 for the
avr target. For this target, zero and offsets from zero are perfectly
valid addresses, and the default value of param_min_pagesize ends up
triggering warnings on valid memory accesses.


I think the proper configuration is to have
DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID


Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID


That worked. Ok for trunk and backporting to 13 and 12 branches
(pending regression testing)?


OK, but please let Denis time to comment.


Didn't hear from Denis. When running regression tests with this patch,
I found that some tests with -fdelete-null-pointer-checks were
failing. Commit 19416210b37db0584cd0b3f3b3961324b8973d25 made
-fdelete-null-pointer-checks false by default, while still allowing it
to be overridden from the command line (it was previously
unconditionally false).

To keep the same behavior, I modified the hook to report zero
addresses as valid only if -fdelete-null-pointer-checks is not set.
With this change, all regression tests pass.

Ok for trunk and backporting to 13 and 12 branches?


I think that's bit backwards - this hook conveys more precise information
(it's address-space specific) and it is also more specific.  Instead I'd
suggest to set the flag to zero in the target like nios2 or msp430 do.
In fact we should probably initialize it using this hook (and using the
default address space).


Does the below patch work? The hook impl reports that zero address is
valid, and flag_delete_null_pointer_checks is set to zero if the
hook says zero is a valid address.

As flag_delete_null_pointer_checks is now always disabled for avr, I
removed the resetting code in avr-common.cc that disables it for
OPT_LEVELS_ALL by default, and added avr as a target that always keeps
null pointer checks in testsuite/lib/target-supports.exp.

I also removed ATTRIBUTE_UNUSED and the parameter name in the target
hook to address https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619014.html.

PR 105523

gcc/ChangeLog:

* common/config/avr/avr-common.cc: Remove setting
of OPT_fdelete_null_pointer_checks.
* config/avr/avr.cc (avr_option_override): Clear
flag_delete_null_pointer_checks if zero_address_valid.
(avr_addr_space_zero_address_valid): New function.
(TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Provide target
hook.

gcc/testsuite/ChangeLog:

* lib/target-supports.exp
(check_effective_target_keeps_null_pointer_checks): Add
avr.
* gcc.target/avr/pr105523.c: New test.

OK for the trunk.

I don't think we're likely to hear from Denis.  I haven't heard from him 
in quite a while, nor has Georg.  So no need to wait.


jeff

[Ping] Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523]

2023-06-16 Thread SenthilKumar.Selvaraj--- via Gcc-patches 
On Fri, 2023-06-02 at 12:32 +0530, Senthil Kumar Selvaraj wrote:
> On Mon, 2023-05-22 at 14:05 +0200, Richard Biener wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the 
> > content is safe
> > 
> > On Fri, May 19, 2023 at 7:58 AM  wrote:
> > > On 26/04/23, 5:51 PM, "Richard Biener"  > > > wrote:
> > > > On Wed, Apr 26, 2023 at 12:56 PM  > > > > wrote:
> > > > > On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches 
> > > > > mailto:gcc-patches@gcc.gnu.org>> wrote:
> > > > > > On Wed, Apr 26, 2023 at 11:42 AM Richard Biener
> > > > > > mailto:richard.guent...@gmail.com>> 
> > > > > > wrote:
> > > > > > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via
> > > > > > > Gcc-patches  > > > > > > > wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > This patch fixes PR 105523 by setting param_min_pagesize to 0 
> > > > > > > > for the
> > > > > > > > avr target. For this target, zero and offsets from zero are 
> > > > > > > > perfectly
> > > > > > > > valid addresses, and the default value of param_min_pagesize 
> > > > > > > > ends up
> > > > > > > > triggering warnings on valid memory accesses.
> > > > > > > 
> > > > > > > I think the proper configuration is to have
> > > > > > > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID
> > > > > > 
> > > > > > Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
> > > > > 
> > > > > That worked. Ok for trunk and backporting to 13 and 12 branches
> > > > > (pending regression testing)?
> > > > 
> > > > OK, but please let Denis time to comment.
> > > 
> > > Didn't hear from Denis. When running regression tests with this patch,
> > > I found that some tests with -fdelete-null-pointer-checks were
> > > failing. Commit 19416210b37db0584cd0b3f3b3961324b8973d25 made
> > > -fdelete-null-pointer-checks false by default, while still allowing it
> > > to be overridden from the command line (it was previously
> > > unconditionally false).
> > > 
> > > To keep the same behavior, I modified the hook to report zero
> > > addresses as valid only if -fdelete-null-pointer-checks is not set.
> > > With this change, all regression tests pass.
> > > 
> > > Ok for trunk and backporting to 13 and 12 branches?
> > 
> > I think that's bit backwards - this hook conveys more precise information
> > (it's address-space specific) and it is also more specific.  Instead I'd
> > suggest to set the flag to zero in the target like nios2 or msp430 do.
> > In fact we should probably initialize it using this hook (and using the
> > default address space).
> 
> Does the below patch work? The hook impl reports that zero address is
> valid, and flag_delete_null_pointer_checks is set to zero if the
> hook says zero is a valid address.
> 
> As flag_delete_null_pointer_checks is now always disabled for avr, I
> removed the resetting code in avr-common.cc that disables it for
> OPT_LEVELS_ALL by default, and added avr as a target that always keeps
> null pointer checks in testsuite/lib/target-supports.exp.
> 
> I also removed ATTRIBUTE_UNUSED and the parameter name in the target
> hook to address 
> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619014.html. 
> 
> PR 105523
> 
> gcc/ChangeLog:
> 
>   * common/config/avr/avr-common.cc: Remove setting
>   of OPT_fdelete_null_pointer_checks.
>   * config/avr/avr.cc (avr_option_override): Clear
>   flag_delete_null_pointer_checks if zero_address_valid.
>   (avr_addr_space_zero_address_valid): New function.
>   (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Provide target
>   hook.
> 
> gcc/testsuite/ChangeLog:
> 
>   * lib/target-supports.exp
>   (check_effective_target_keeps_null_pointer_checks): Add
>   avr.
>   * gcc.target/avr/pr105523.c: New test.
> 
> diff --git gcc/common/config/avr/avr-common.cc 
> gcc/common/config/avr/avr-common.cc
> index 2ad0244..2f874c5 100644
> --- gcc/common/config/avr/avr-common.cc
> +++ gcc/common/config/avr/avr-common.cc
> @@ -29,12 +29,6 @@
>  /* Implement TARGET_OPTION_OPTIMIZATION_TABLE.  */
>  static const struct default_options avr_option_optimization_table[] =
>{
> -// With -fdelete-null-pointer-checks option, the compiler assumes
> -// that dereferencing of a null pointer would halt the program.
> -// For AVR this assumption is not true and a program can safely
> -// dereference null pointers.  Changes made by this option may not
> -// work properly for AVR.  So disable this option.
> -{ OPT_LEVELS_ALL, OPT_fdelete_null_pointer_checks, NULL, 0 },
>  // The only effect of -fcaller-saves might be that it triggers
>  // a frame without need when it tries to be smart around calls.
>  { OPT_LEVELS_ALL, OPT_fcaller_saves, NULL, 0 },
> diff --git gcc/config/avr/avr.cc gcc/config/avr/avr.cc
> index a90cade..b987837 100644
> --- gcc/config/avr/avr.cc
> +++ gcc/config/avr/avr.cc
> @@ -756,6 +756,10 @@ av

Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523]

2023-06-02 Thread SenthilKumar.Selvaraj--- via Gcc-patches 
On Mon, 2023-05-22 at 14:05 +0200, Richard Biener wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the 
> content is safe
> 
> On Fri, May 19, 2023 at 7:58 AM  wrote:
> > On 26/04/23, 5:51 PM, "Richard Biener"  > > wrote:
> > > On Wed, Apr 26, 2023 at 12:56 PM  > > > wrote:
> > > > On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches 
> > > > mailto:gcc-patches@gcc.gnu.org>> wrote:
> > > > > On Wed, Apr 26, 2023 at 11:42 AM Richard Biener
> > > > > mailto:richard.guent...@gmail.com>> 
> > > > > wrote:
> > > > > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via
> > > > > > Gcc-patches  > > > > > > wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for 
> > > > > > > the
> > > > > > > avr target. For this target, zero and offsets from zero are 
> > > > > > > perfectly
> > > > > > > valid addresses, and the default value of param_min_pagesize ends 
> > > > > > > up
> > > > > > > triggering warnings on valid memory accesses.
> > > > > > 
> > > > > > I think the proper configuration is to have
> > > > > > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID
> > > > > 
> > > > > Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
> > > > 
> > > > That worked. Ok for trunk and backporting to 13 and 12 branches
> > > > (pending regression testing)?
> > > 
> > > OK, but please let Denis time to comment.
> > 
> > Didn't hear from Denis. When running regression tests with this patch,
> > I found that some tests with -fdelete-null-pointer-checks were
> > failing. Commit 19416210b37db0584cd0b3f3b3961324b8973d25 made
> > -fdelete-null-pointer-checks false by default, while still allowing it
> > to be overridden from the command line (it was previously
> > unconditionally false).
> > 
> > To keep the same behavior, I modified the hook to report zero
> > addresses as valid only if -fdelete-null-pointer-checks is not set.
> > With this change, all regression tests pass.
> > 
> > Ok for trunk and backporting to 13 and 12 branches?
> 
> I think that's bit backwards - this hook conveys more precise information
> (it's address-space specific) and it is also more specific.  Instead I'd
> suggest to set the flag to zero in the target like nios2 or msp430 do.
> In fact we should probably initialize it using this hook (and using the
> default address space).

Does the below patch work? The hook impl reports that zero address is
valid, and flag_delete_null_pointer_checks is set to zero if the
hook says zero is a valid address.

As flag_delete_null_pointer_checks is now always disabled for avr, I
removed the resetting code in avr-common.cc that disables it for
OPT_LEVELS_ALL by default, and added avr as a target that always keeps
null pointer checks in testsuite/lib/target-supports.exp.

I also removed ATTRIBUTE_UNUSED and the parameter name in the target
hook to address https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619014.html. 

PR 105523

gcc/ChangeLog:

* common/config/avr/avr-common.cc: Remove setting
of OPT_fdelete_null_pointer_checks.
* config/avr/avr.cc (avr_option_override): Clear
flag_delete_null_pointer_checks if zero_address_valid.
(avr_addr_space_zero_address_valid): New function.
(TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Provide target
hook.

gcc/testsuite/ChangeLog:

* lib/target-supports.exp
(check_effective_target_keeps_null_pointer_checks): Add
avr.
* gcc.target/avr/pr105523.c: New test.

diff --git gcc/common/config/avr/avr-common.cc 
gcc/common/config/avr/avr-common.cc
index 2ad0244..2f874c5 100644
--- gcc/common/config/avr/avr-common.cc
+++ gcc/common/config/avr/avr-common.cc
@@ -29,12 +29,6 @@
 /* Implement TARGET_OPTION_OPTIMIZATION_TABLE.  */
 static const struct default_options avr_option_optimization_table[] =
   {
-// With -fdelete-null-pointer-checks option, the compiler assumes
-// that dereferencing of a null pointer would halt the program.
-// For AVR this assumption is not true and a program can safely
-// dereference null pointers.  Changes made by this option may not
-// work properly for AVR.  So disable this option.
-{ OPT_LEVELS_ALL, OPT_fdelete_null_pointer_checks, NULL, 0 },
 // The only effect of -fcaller-saves might be that it triggers
 // a frame without need when it tries to be smart around calls.
 { OPT_LEVELS_ALL, OPT_fcaller_saves, NULL, 0 },
diff --git gcc/config/avr/avr.cc gcc/config/avr/avr.cc
index a90cade..b987837 100644
--- gcc/config/avr/avr.cc
+++ gcc/config/avr/avr.cc
@@ -756,6 +756,10 @@ avr_option_override (void)
   flag_omit_frame_pointer = 0;
 }
 
+  /* Disable flag_delete_null_pointer_checks if zero is a valid address. */
+  if (targetm.addr_space.zero_address_valid (ADDR_SPACE_GENERIC))
+flag_delete_null_pointer_checks = 0;
+
   if (f

Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523]

2023-05-22 Thread Richard Biener via Gcc-patches 
On Fri, May 19, 2023 at 7:58 AM  wrote:
>
> On 26/04/23, 5:51 PM, "Richard Biener"  > wrote:
> > On Wed, Apr 26, 2023 at 12:56 PM  > > wrote:
> > >
> > > On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches 
> > > mailto:gcc-patches@gcc.gnu.org>> wrote:
> > > >
> > > > On Wed, Apr 26, 2023 at 11:42 AM Richard Biener
> > > > mailto:richard.guent...@gmail.com>> wrote:
> > > > >
> > > > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via
> > > > > Gcc-patches  > > > > > wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for 
> > > > > > the
> > > > > > avr target. For this target, zero and offsets from zero are 
> > > > > > perfectly
> > > > > > valid addresses, and the default value of param_min_pagesize ends up
> > > > > > triggering warnings on valid memory accesses.
> > > > >
> > > > > I think the proper configuration is to have
> > > > > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID
> > > >
> > > > Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
> > >
> > > That worked. Ok for trunk and backporting to 13 and 12 branches
> > > (pending regression testing)?
> >
> >
> > OK, but please let Denis time to comment.
>
> Didn't hear from Denis. When running regression tests with this patch,
> I found that some tests with -fdelete-null-pointer-checks were
> failing. Commit 19416210b37db0584cd0b3f3b3961324b8973d25 made
> -fdelete-null-pointer-checks false by default, while still allowing it
> to be overridden from the command line (it was previously
> unconditionally false).
>
> To keep the same behavior, I modified the hook to report zero
> addresses as valid only if -fdelete-null-pointer-checks is not set.
> With this change, all regression tests pass.
>
> Ok for trunk and backporting to 13 and 12 branches?

I think that's bit backwards - this hook conveys more precise information
(it's address-space specific) and it is also more specific.  Instead I'd
suggest to set the flag to zero in the target like nios2 or msp430 do.
In fact we should probably initialize it using this hook (and using the
default address space).

Richard.

> Regards
> Senthil
>
> PR 105523
>
> gcc/ChangeLog:
>
> * config/avr/avr.cc (avr_addr_space_zero_address_valid):
> (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Return true if
> flag_delete_null_pointer_checks is not set.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/avr/pr105523.c: New test.
>
>
> diff --git gcc/config/avr/avr.cc gcc/config/avr/avr.cc
> index d5af40f..4c9eb84 100644
> --- gcc/config/avr/avr.cc
> +++ gcc/config/avr/avr.cc
> @@ -9787,6 +9787,18 @@ avr_addr_space_diagnose_usage (addr_space_t as, 
> location_t loc)
>(void) avr_addr_space_supported_p (as, loc);
>  }
>
> +/* 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, where RAMPZ is
> +   set to the appropriate segment value.
> +   If the user explicitly passes in -fdelete-null-pointer-checks though,
> +   assume zero addresses are invalid.*/
> +
> +static bool
> +avr_addr_space_zero_address_valid (addr_space_t as ATTRIBUTE_UNUSED)
> +{
> +  return flag_delete_null_pointer_checks == 0;
> +}
>
>  /* Look if DECL shall be placed in program memory space by
> means of attribute `progmem' or some address-space qualifier.
> @@ -14687,6 +14699,9 @@ avr_float_lib_compare_returns_bool (machine_mode 
> mode, enum rtx_code)
>  #undef  TARGET_ADDR_SPACE_DIAGNOSE_USAGE
>  #define TARGET_ADDR_SPACE_DIAGNOSE_USAGE avr_addr_space_diagnose_usage
>
> +#undef  TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
> +#define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID 
> avr_addr_space_zero_address_valid
> +
>  #undef  TARGET_MODE_DEPENDENT_ADDRESS_P
>  #define TARGET_MODE_DEPENDENT_ADDRESS_P avr_mode_dependent_address_p
>
> diff --git gcc/testsuite/gcc.target/avr/pr105523.c 
> gcc/testsuite/gcc.target/avr/pr105523.c
> new file mode 100644
> index 000..fbbf7bf
> --- /dev/null
> +++ gcc/testsuite/gcc.target/avr/pr105523.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Os -Wall" } */
> +
> +/* Verify no "array subscript 0 is outside array bounds of" is generated
> +   for accessing memory addresses in the 0-4096 range. */
> +
> +typedef __UINT8_TYPE__ uint8_t;
> +
> +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ ))
> +
> +void bar (void)
> +{
> +SREG = 0;
> +}
>

Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523]

2023-05-19 Thread Jeff Law via Gcc-patches 




On 5/19/23 08:02, Bernhard Reutner-Fischer via Gcc-patches wrote:

On 19 May 2023 07:58:48 CEST, "SenthilKumar.Selvaraj--- via Gcc-patches" 
 wrote:

Just a nit:


+static bool
+avr_addr_space_zero_address_valid (addr_space_t as ATTRIBUTE_UNUSED)
+{
+  return flag_delete_null_pointer_checks == 0;
+}


Since we are c++ nowadays, you can omit the parameter name for unused 
arguments. I.e.:

static bool
avr_addr_space_zero_address_valid (addr_space_t)
{

Right.  And I strongly prefer that over ATTRIBUTE_UNUSED.

So OK for the trunk with that change.

jeff

Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523]

2023-05-19 Thread Bernhard Reutner-Fischer via Gcc-patches 
On 19 May 2023 07:58:48 CEST, "SenthilKumar.Selvaraj--- via Gcc-patches" 
 wrote:

Just a nit:

>+static bool
>+avr_addr_space_zero_address_valid (addr_space_t as ATTRIBUTE_UNUSED)
>+{
>+  return flag_delete_null_pointer_checks == 0;
>+}

Since we are c++ nowadays, you can omit the parameter name for unused 
arguments. I.e.:

static bool
avr_addr_space_zero_address_valid (addr_space_t)
{
  ...

Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523]

2023-05-18 Thread SenthilKumar.Selvaraj--- via Gcc-patches 
On 26/04/23, 5:51 PM, "Richard Biener" mailto:richard.guent...@gmail.com>> wrote:
> On Wed, Apr 26, 2023 at 12:56 PM  > wrote:
> >
> > On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches 
> > mailto:gcc-patches@gcc.gnu.org>> wrote:
> > >
> > > On Wed, Apr 26, 2023 at 11:42 AM Richard Biener
> > > mailto:richard.guent...@gmail.com>> wrote:
> > > >
> > > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via
> > > > Gcc-patches mailto:gcc-patches@gcc.gnu.org>> 
> > > > wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for the
> > > > > avr target. For this target, zero and offsets from zero are perfectly
> > > > > valid addresses, and the default value of param_min_pagesize ends up
> > > > > triggering warnings on valid memory accesses.
> > > >
> > > > I think the proper configuration is to have
> > > > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID
> > >
> > > Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
> >
> > That worked. Ok for trunk and backporting to 13 and 12 branches
> > (pending regression testing)?
> 
> 
> OK, but please let Denis time to comment.

Didn't hear from Denis. When running regression tests with this patch,
I found that some tests with -fdelete-null-pointer-checks were
failing. Commit 19416210b37db0584cd0b3f3b3961324b8973d25 made
-fdelete-null-pointer-checks false by default, while still allowing it
to be overridden from the command line (it was previously
unconditionally false).

To keep the same behavior, I modified the hook to report zero
addresses as valid only if -fdelete-null-pointer-checks is not set.
With this change, all regression tests pass.

Ok for trunk and backporting to 13 and 12 branches?

Regards
Senthil

PR 105523

gcc/ChangeLog:

* config/avr/avr.cc (avr_addr_space_zero_address_valid):
(TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Return true if
flag_delete_null_pointer_checks is not set.

gcc/testsuite/ChangeLog:

* gcc.target/avr/pr105523.c: New test.


diff --git gcc/config/avr/avr.cc gcc/config/avr/avr.cc
index d5af40f..4c9eb84 100644
--- gcc/config/avr/avr.cc
+++ gcc/config/avr/avr.cc
@@ -9787,6 +9787,18 @@ avr_addr_space_diagnose_usage (addr_space_t as, 
location_t loc)
   (void) avr_addr_space_supported_p (as, loc);
 }
 
+/* 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, where RAMPZ is
+   set to the appropriate segment value.
+   If the user explicitly passes in -fdelete-null-pointer-checks though,
+   assume zero addresses are invalid.*/
+
+static bool
+avr_addr_space_zero_address_valid (addr_space_t as ATTRIBUTE_UNUSED)
+{
+  return flag_delete_null_pointer_checks == 0;
+}
 
 /* Look if DECL shall be placed in program memory space by
means of attribute `progmem' or some address-space qualifier.
@@ -14687,6 +14699,9 @@ avr_float_lib_compare_returns_bool (machine_mode mode, 
enum rtx_code)
 #undef  TARGET_ADDR_SPACE_DIAGNOSE_USAGE
 #define TARGET_ADDR_SPACE_DIAGNOSE_USAGE avr_addr_space_diagnose_usage
 
+#undef  TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
+#define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID avr_addr_space_zero_address_valid
+
 #undef  TARGET_MODE_DEPENDENT_ADDRESS_P
 #define TARGET_MODE_DEPENDENT_ADDRESS_P avr_mode_dependent_address_p
 
diff --git gcc/testsuite/gcc.target/avr/pr105523.c 
gcc/testsuite/gcc.target/avr/pr105523.c
new file mode 100644
index 000..fbbf7bf
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr105523.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -Wall" } */
+
+/* Verify no "array subscript 0 is outside array bounds of" is generated
+   for accessing memory addresses in the 0-4096 range. */
+
+typedef __UINT8_TYPE__ uint8_t;
+
+#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ ))
+
+void bar (void)
+{
+SREG = 0;
+}

Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523]

2023-04-26 Thread Richard Biener via Gcc-patches 
On Wed, Apr 26, 2023 at 12:56 PM  wrote:
>
> On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches 
>  wrote:
> >
> > On Wed, Apr 26, 2023 at 11:42 AM Richard Biener
> >  wrote:
> > >
> > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via
> > > Gcc-patches  wrote:
> > > >
> > > > Hi,
> > > >
> > > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for the
> > > > avr target. For this target, zero and offsets from zero are perfectly
> > > > valid addresses, and the default value of param_min_pagesize ends up
> > > > triggering warnings on valid memory accesses.
> > >
> > > I think the proper configuration is to have
> > > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID
> >
> > Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
>
> That worked. Ok for trunk and backporting to 13 and 12 branches
> (pending regression testing)?

OK, but please let Denis time to comment.

Richard.

> Regards,
> Senthil
>
> PR 105523
>
> gcc/ChangeLog:
>
> * config/avr/avr.cc (avr_addr_space_zero_address_valid):
> (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Return true.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/avr/pr105523.c: New test.
>
>
>
> diff --git a/gcc/config/avr/avr.cc b/gcc/config/avr/avr.cc
> index c193430cf07..5439eb8e55c 100644
> --- a/gcc/config/avr/avr.cc
> +++ b/gcc/config/avr/avr.cc
> @@ -9788,6 +9788,16 @@ avr_addr_space_diagnose_usage (addr_space_t as, 
> location_t loc)
>(void) avr_addr_space_supported_p (as, loc);
>  }
>
> +/* 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, where RAMPZ is
> +   set to the appropriate segment value. */
> +
> +static bool
> +avr_addr_space_zero_address_valid (addr_space_t as)
> +{
> +  return true;
> +}
>
>  /* Look if DECL shall be placed in program memory space by
> means of attribute `progmem' or some address-space qualifier.
> @@ -14688,6 +14698,9 @@ avr_float_lib_compare_returns_bool (machine_mode 
> mode, enum rtx_code)
>  #undef  TARGET_ADDR_SPACE_DIAGNOSE_USAGE
>  #define TARGET_ADDR_SPACE_DIAGNOSE_USAGE avr_addr_space_diagnose_usage
>
> +#undef  TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
> +#define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID 
> avr_addr_space_zero_address_valid
> +
>  #undef  TARGET_MODE_DEPENDENT_ADDRESS_P
>  #define TARGET_MODE_DEPENDENT_ADDRESS_P avr_mode_dependent_address_p
>
> diff --git a/gcc/testsuite/gcc.target/avr/pr105523.c 
> b/gcc/testsuite/gcc.target/avr/pr105523.c
> new file mode 100644
> index 000..fbbf7bf4422
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/avr/pr105523.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Os -Wall" } */
> +
> +/* Verify no "array subscript 0 is outside array bounds of" is generated
> +   for accessing memory addresses in the 0-4096 range. */
> +
> +typedef __UINT8_TYPE__ uint8_t;
> +
> +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ ))
> +
> +void bar (void)
> +{
> +SREG = 0;
> +}
>
>

Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523]

2023-04-26 Thread SenthilKumar.Selvaraj--- via Gcc-patches 
On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches 
 wrote:
>
> On Wed, Apr 26, 2023 at 11:42 AM Richard Biener
>  wrote:
> >
> > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via
> > Gcc-patches  wrote:
> > >
> > > Hi,
> > >
> > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for the
> > > avr target. For this target, zero and offsets from zero are perfectly
> > > valid addresses, and the default value of param_min_pagesize ends up
> > > triggering warnings on valid memory accesses.
> >
> > I think the proper configuration is to have
> > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID
>
> Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID

That worked. Ok for trunk and backporting to 13 and 12 branches
(pending regression testing)?

Regards,
Senthil

PR 105523

gcc/ChangeLog:

* config/avr/avr.cc (avr_addr_space_zero_address_valid):
(TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Return true.

gcc/testsuite/ChangeLog:

* gcc.target/avr/pr105523.c: New test.



diff --git a/gcc/config/avr/avr.cc b/gcc/config/avr/avr.cc
index c193430cf07..5439eb8e55c 100644
--- a/gcc/config/avr/avr.cc
+++ b/gcc/config/avr/avr.cc
@@ -9788,6 +9788,16 @@ avr_addr_space_diagnose_usage (addr_space_t as, 
location_t loc)
   (void) avr_addr_space_supported_p (as, loc);
 }
 
+/* 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, where RAMPZ is
+   set to the appropriate segment value. */
+
+static bool
+avr_addr_space_zero_address_valid (addr_space_t as)
+{
+  return true;
+}
 
 /* Look if DECL shall be placed in program memory space by
means of attribute `progmem' or some address-space qualifier.
@@ -14688,6 +14698,9 @@ avr_float_lib_compare_returns_bool (machine_mode mode, 
enum rtx_code)
 #undef  TARGET_ADDR_SPACE_DIAGNOSE_USAGE
 #define TARGET_ADDR_SPACE_DIAGNOSE_USAGE avr_addr_space_diagnose_usage
 
+#undef  TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
+#define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID avr_addr_space_zero_address_valid
+
 #undef  TARGET_MODE_DEPENDENT_ADDRESS_P
 #define TARGET_MODE_DEPENDENT_ADDRESS_P avr_mode_dependent_address_p
 
diff --git a/gcc/testsuite/gcc.target/avr/pr105523.c 
b/gcc/testsuite/gcc.target/avr/pr105523.c
new file mode 100644
index 000..fbbf7bf4422
--- /dev/null
+++ b/gcc/testsuite/gcc.target/avr/pr105523.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -Wall" } */
+
+/* Verify no "array subscript 0 is outside array bounds of" is generated
+   for accessing memory addresses in the 0-4096 range. */
+
+typedef __UINT8_TYPE__ uint8_t;
+
+#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ ))
+
+void bar (void)
+{
+SREG = 0;
+}

Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523]

2023-04-26 Thread Richard Biener via Gcc-patches 
On Wed, Apr 26, 2023 at 11:42 AM Richard Biener
 wrote:
>
> On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via
> Gcc-patches  wrote:
> >
> > Hi,
> >
> > This patch fixes PR 105523 by setting param_min_pagesize to 0 for the
> > avr target. For this target, zero and offsets from zero are perfectly
> > valid addresses, and the default value of param_min_pagesize ends up
> > triggering warnings on valid memory accesses.
>
> I think the proper configuration is to have
> DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID

Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID

> defined to something returning true instead.
>
> Richard.
>
> > Ok for trunk and backporting to 13 and 12 branches?
> >
> > Regards
> > Senthil
> >
> > PR target/105523
> >
> > gcc/ChangeLog:
> >
> > * config/avr/avr.cc (avr_option_override): Set
> > param_min_pagesize to 0.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/avr/pr105523.c: New test.
> >
> > diff --git a/gcc/config/avr/avr.cc b/gcc/config/avr/avr.cc
> > index c193430cf07..3b862f4e4ac 100644
> > --- a/gcc/config/avr/avr.cc
> > +++ b/gcc/config/avr/avr.cc
> > @@ -56,6 +56,7 @@
> >  #include "tree-pass.h"
> >  #include "print-rtl.h"
> >  #include "rtl-iter.h"
> > +#include "opts.h"
> >
> >  /* This file should be included last.  */
> >  #include "target-def.h"
> > @@ -769,6 +770,9 @@ avr_option_override (void)
> >avr_gasisr_prologues = 0;
> >  #endif
> >
> > +  SET_OPTION_IF_UNSET (&global_options, &global_options_set,
> > +   param_min_pagesize, 0);
> > +
> >if (!avr_set_core_architecture())
> >  return;
> >
> > diff --git a/gcc/testsuite/gcc.target/avr/pr105523.c 
> > b/gcc/testsuite/gcc.target/avr/pr105523.c
> > new file mode 100644
> > index 000..fbbf7bf4422
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/avr/pr105523.c
> > @@ -0,0 +1,14 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-Os -Wall" } */
> > +
> > +/* Verify no "array subscript 0 is outside array bounds of" is generated
> > +   for accessing memory addresses in the 0-4096 range. */
> > +
> > +typedef __UINT8_TYPE__ uint8_t;
> > +
> > +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ ))
> > +
> > +void bar (void)
> > +{
> > +SREG = 0;
> > +}
> >

Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523]

2023-04-26 Thread Richard Biener via Gcc-patches 
On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via
Gcc-patches  wrote:
>
> Hi,
>
> This patch fixes PR 105523 by setting param_min_pagesize to 0 for the
> avr target. For this target, zero and offsets from zero are perfectly
> valid addresses, and the default value of param_min_pagesize ends up
> triggering warnings on valid memory accesses.

I think the proper configuration is to have
DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID
defined to something returning true instead.

Richard.

> Ok for trunk and backporting to 13 and 12 branches?
>
> Regards
> Senthil
>
> PR target/105523
>
> gcc/ChangeLog:
>
> * config/avr/avr.cc (avr_option_override): Set
> param_min_pagesize to 0.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/avr/pr105523.c: New test.
>
> diff --git a/gcc/config/avr/avr.cc b/gcc/config/avr/avr.cc
> index c193430cf07..3b862f4e4ac 100644
> --- a/gcc/config/avr/avr.cc
> +++ b/gcc/config/avr/avr.cc
> @@ -56,6 +56,7 @@
>  #include "tree-pass.h"
>  #include "print-rtl.h"
>  #include "rtl-iter.h"
> +#include "opts.h"
>
>  /* This file should be included last.  */
>  #include "target-def.h"
> @@ -769,6 +770,9 @@ avr_option_override (void)
>avr_gasisr_prologues = 0;
>  #endif
>
> +  SET_OPTION_IF_UNSET (&global_options, &global_options_set,
> +   param_min_pagesize, 0);
> +
>if (!avr_set_core_architecture())
>  return;
>
> diff --git a/gcc/testsuite/gcc.target/avr/pr105523.c 
> b/gcc/testsuite/gcc.target/avr/pr105523.c
> new file mode 100644
> index 000..fbbf7bf4422
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/avr/pr105523.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Os -Wall" } */
> +
> +/* Verify no "array subscript 0 is outside array bounds of" is generated
> +   for accessing memory addresses in the 0-4096 range. */
> +
> +typedef __UINT8_TYPE__ uint8_t;
> +
> +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ ))
> +
> +void bar (void)
> +{
> +SREG = 0;
> +}
>