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

[Richard Biener <richard.guenther@gmail.com>]

On Fri, May 19, 2023 at 7:58 AM <SenthilKumar.Selvaraj@microchip.com> wrote:
>
> On 26/04/23, 5:51 PM, "Richard Biener" <richard.guenther@gmail.com <mailto:richard.guenther@gmail.com>> wrote:
> > On Wed, Apr 26, 2023 at 12:56 PM <SenthilKumar.Selvaraj@microchip.com <mailto:SenthilKumar.Selvaraj@microchip.com>> wrote:
> > >
> > > On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote:
> > > >
> > > > On Wed, Apr 26, 2023 at 11:42 AM Richard Biener
> > > > <richard.guenther@gmail.com <mailto:richard.guenther@gmail.com>> wrote:
> > > > >
> > > > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via
> > > > > Gcc-patches <gcc-patches@gcc.gnu.org <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).

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<RAMPZ val>0000, 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 0000000..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;
> +}
>