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

[Jeff Law <jeffreyalaw@gmail.com>]

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 <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).
>>
>> 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