diff mbox series

[1/2] select .rodata for const volatile variables.

Message ID 20221202175225.2780-2-cupertino.miranda@oracle.com
State Committed
Commit 011c0c29a2452e588f255673238460da4167c4c0
Headers
Series [1/2] select .rodata for const volatile variables. |

Commit Message

Cupertino Miranda Dec. 2, 2022, 5:52 p.m. UTC 
  Changed target code to select .rodata section for 'const volatile'
defined variables.
This change is in the context of the bugzilla #170181.

gcc/ChangeLog:

	v850.c(v850_select_section): Changed function.
---
 gcc/config/v850/v850.cc | 1 -
 1 file changed, 1 deletion(-)

Comments

Jeff Law Dec. 5, 2022, 6:06 p.m. UTC | #1 
On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
> Changed target code to select .rodata section for 'const volatile'
> defined variables.
> This change is in the context of the bugzilla #170181.
> 
> gcc/ChangeLog:
> 
> 	v850.c(v850_select_section): Changed function.
I'm not sure this is safe/correct.  ISTM that you need to look at the 
underlying TREE_TYPE to check for const-volatile rather than 
TREE_SIDE_EFFECTS.

Of secondary importance is the ChangeLog.  Just saying "Changed 
function" provides no real information.  Something like this would be 
better:

	* config/v850/v850.c (v850_select_section): Put const volatile
	objects into read-only sections.


Jeff




> ---
>   gcc/config/v850/v850.cc | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
> index c7d432990ab..e66893fede4 100644
> --- a/gcc/config/v850/v850.cc
> +++ b/gcc/config/v850/v850.cc
> @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>       {
>         int is_const;
>         if (!TREE_READONLY (exp)
> -	  || TREE_SIDE_EFFECTS (exp)
>   	  || !DECL_INITIAL (exp)
>   	  || (DECL_INITIAL (exp) != error_mark_node
>   	      && !TREE_CONSTANT (DECL_INITIAL (exp))))
Cupertino Miranda Dec. 7, 2022, 3:12 p.m. UTC | #2 
Hi Jeff,

First of all thanks for your quick review.
Apologies for the delay replying, the message got lost in my inbox.

> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>> Changed target code to select .rodata section for 'const volatile'
>> defined variables.
>> This change is in the context of the bugzilla #170181.
>> gcc/ChangeLog:
>> 	v850.c(v850_select_section): Changed function.
> I'm not sure this is safe/correct.  ISTM that you need to look at the underlying
> TREE_TYPE to check for const-volatile rather than TREE_SIDE_EFFECTS.

I believe this was asked by Jose when he first sent the generic patches.
Please notice my change is influenced by his original patch that does
the same and was approved.

https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599348.html
https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602374.html

>
> Of secondary importance is the ChangeLog.  Just saying "Changed function"
> provides no real information.  Something like this would be better:
>
> 	* config/v850/v850.c (v850_select_section): Put const volatile
> 	objects into read-only sections.
>
>
> Jeff
>
>
>
>
>> ---
>>   gcc/config/v850/v850.cc | 1 -
>>   1 file changed, 1 deletion(-)
>> diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
>> index c7d432990ab..e66893fede4 100644
>> --- a/gcc/config/v850/v850.cc
>> +++ b/gcc/config/v850/v850.cc
>> @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>>       {
>>         int is_const;
>>         if (!TREE_READONLY (exp)
>> -	  || TREE_SIDE_EFFECTS (exp)
>>   	  || !DECL_INITIAL (exp)
>>   	  || (DECL_INITIAL (exp) != error_mark_node
>>   	      && !TREE_CONSTANT (DECL_INITIAL (exp))))
Cupertino Miranda Dec. 15, 2022, 10:13 a.m. UTC | #3 
gentle ping

Cupertino Miranda writes:

> Hi Jeff,
>
> First of all thanks for your quick review.
> Apologies for the delay replying, the message got lost in my inbox.
>
>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>> Changed target code to select .rodata section for 'const volatile'
>>> defined variables.
>>> This change is in the context of the bugzilla #170181.
>>> gcc/ChangeLog:
>>> 	v850.c(v850_select_section): Changed function.
>> I'm not sure this is safe/correct.  ISTM that you need to look at the underlying
>> TREE_TYPE to check for const-volatile rather than TREE_SIDE_EFFECTS.
>
> I believe this was asked by Jose when he first sent the generic patches.
> Please notice my change is influenced by his original patch that does
> the same and was approved.
>
> https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599348.html
> https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602374.html
>
>>
>> Of secondary importance is the ChangeLog.  Just saying "Changed function"
>> provides no real information.  Something like this would be better:
>>
>> 	* config/v850/v850.c (v850_select_section): Put const volatile
>> 	objects into read-only sections.
>>
>>
>> Jeff
>>
>>
>>
>>
>>> ---
>>>   gcc/config/v850/v850.cc | 1 -
>>>   1 file changed, 1 deletion(-)
>>> diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
>>> index c7d432990ab..e66893fede4 100644
>>> --- a/gcc/config/v850/v850.cc
>>> +++ b/gcc/config/v850/v850.cc
>>> @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>>>       {
>>>         int is_const;
>>>         if (!TREE_READONLY (exp)
>>> -	  || TREE_SIDE_EFFECTS (exp)
>>>   	  || !DECL_INITIAL (exp)
>>>   	  || (DECL_INITIAL (exp) != error_mark_node
>>>   	      && !TREE_CONSTANT (DECL_INITIAL (exp))))
Cupertino Miranda Dec. 22, 2022, 5:21 p.m. UTC | #4 
Cupertino Miranda via Gcc-patches writes:

> gentle ping
>
> Cupertino Miranda writes:
>
>> Hi Jeff,
>>
>> First of all thanks for your quick review.
>> Apologies for the delay replying, the message got lost in my inbox.
>>
>>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>>> Changed target code to select .rodata section for 'const volatile'
>>>> defined variables.
>>>> This change is in the context of the bugzilla #170181.
>>>> gcc/ChangeLog:
>>>> 	v850.c(v850_select_section): Changed function.
>>> I'm not sure this is safe/correct.  ISTM that you need to look at the underlying
>>> TREE_TYPE to check for const-volatile rather than TREE_SIDE_EFFECTS.
>>
>> I believe this was asked by Jose when he first sent the generic patches.
>> Please notice my change is influenced by his original patch that does
>> the same and was approved.
>>
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599348.html
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602374.html
>>
>>>
>>> Of secondary importance is the ChangeLog.  Just saying "Changed function"
>>> provides no real information.  Something like this would be better:
>>>
>>> 	* config/v850/v850.c (v850_select_section): Put const volatile
>>> 	objects into read-only sections.
>>>
>>>
>>> Jeff
>>>
>>>
>>>
>>>
>>>> ---
>>>>   gcc/config/v850/v850.cc | 1 -
>>>>   1 file changed, 1 deletion(-)
>>>> diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
>>>> index c7d432990ab..e66893fede4 100644
>>>> --- a/gcc/config/v850/v850.cc
>>>> +++ b/gcc/config/v850/v850.cc
>>>> @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>>>>       {
>>>>         int is_const;
>>>>         if (!TREE_READONLY (exp)
>>>> -	  || TREE_SIDE_EFFECTS (exp)
>>>>   	  || !DECL_INITIAL (exp)
>>>>   	  || (DECL_INITIAL (exp) != error_mark_node
>>>>   	      && !TREE_CONSTANT (DECL_INITIAL (exp))))
Cupertino Miranda Jan. 2, 2023, 10:42 a.m. UTC | #5 
PING PING

Cupertino Miranda writes:

> Cupertino Miranda via Gcc-patches writes:
>
>> gentle ping
>>
>> Cupertino Miranda writes:
>>
>>> Hi Jeff,
>>>
>>> First of all thanks for your quick review.
>>> Apologies for the delay replying, the message got lost in my inbox.
>>>
>>>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>>>> Changed target code to select .rodata section for 'const volatile'
>>>>> defined variables.
>>>>> This change is in the context of the bugzilla #170181.
>>>>> gcc/ChangeLog:
>>>>> 	v850.c(v850_select_section): Changed function.
>>>> I'm not sure this is safe/correct.  ISTM that you need to look at the underlying
>>>> TREE_TYPE to check for const-volatile rather than TREE_SIDE_EFFECTS.
>>>
>>> I believe this was asked by Jose when he first sent the generic patches.
>>> Please notice my change is influenced by his original patch that does
>>> the same and was approved.
>>>
>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599348.html
>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602374.html
>>>
>>>>
>>>> Of secondary importance is the ChangeLog.  Just saying "Changed function"
>>>> provides no real information.  Something like this would be better:
>>>>
>>>> 	* config/v850/v850.c (v850_select_section): Put const volatile
>>>> 	objects into read-only sections.
>>>>
>>>>
>>>> Jeff
>>>>
>>>>
>>>>
>>>>
>>>>> ---
>>>>>   gcc/config/v850/v850.cc | 1 -
>>>>>   1 file changed, 1 deletion(-)
>>>>> diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
>>>>> index c7d432990ab..e66893fede4 100644
>>>>> --- a/gcc/config/v850/v850.cc
>>>>> +++ b/gcc/config/v850/v850.cc
>>>>> @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>>>>>       {
>>>>>         int is_const;
>>>>>         if (!TREE_READONLY (exp)
>>>>> -	  || TREE_SIDE_EFFECTS (exp)
>>>>>   	  || !DECL_INITIAL (exp)
>>>>>   	  || (DECL_INITIAL (exp) != error_mark_node
>>>>>   	      && !TREE_CONSTANT (DECL_INITIAL (exp))))
Richard Biener Jan. 9, 2023, 7:57 a.m. UTC | #6 
On Mon, Dec 5, 2022 at 7:07 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
> > Changed target code to select .rodata section for 'const volatile'
> > defined variables.
> > This change is in the context of the bugzilla #170181.
> >
> > gcc/ChangeLog:
> >
> >       v850.c(v850_select_section): Changed function.
> I'm not sure this is safe/correct.  ISTM that you need to look at the
> underlying TREE_TYPE to check for const-volatile rather than
> TREE_SIDE_EFFECTS.

Just to quote tree.h:

/* In any expression, decl, or constant, nonzero means it has side effects or
   reevaluation of the whole expression could produce a different value.
   This is set if any subexpression is a function call, a side effect or a
   reference to a volatile variable.  In a ..._DECL, this is set only if the
   declaration said `volatile'.  This will never be set for a constant.  */
#define TREE_SIDE_EFFECTS(NODE) \
  (NON_TYPE_CHECK (NODE)->base.side_effects_flag)

so if exp is a decl then that's the volatile check.

> Of secondary importance is the ChangeLog.  Just saying "Changed
> function" provides no real information.  Something like this would be
> better:
>
>         * config/v850/v850.c (v850_select_section): Put const volatile
>         objects into read-only sections.
>
>
> Jeff
>
>
>
>
> > ---
> >   gcc/config/v850/v850.cc | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
> > index c7d432990ab..e66893fede4 100644
> > --- a/gcc/config/v850/v850.cc
> > +++ b/gcc/config/v850/v850.cc
> > @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
> >       {
> >         int is_const;
> >         if (!TREE_READONLY (exp)
> > -       || TREE_SIDE_EFFECTS (exp)
> >         || !DECL_INITIAL (exp)
> >         || (DECL_INITIAL (exp) != error_mark_node
> >             && !TREE_CONSTANT (DECL_INITIAL (exp))))
Cupertino Miranda Jan. 13, 2023, 3:06 p.m. UTC | #7 
Richard Biener writes:

> On Mon, Dec 5, 2022 at 7:07 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>>
>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>> > Changed target code to select .rodata section for 'const volatile'
>> > defined variables.
>> > This change is in the context of the bugzilla #170181.
>> >
>> > gcc/ChangeLog:
>> >
>> >       v850.c(v850_select_section): Changed function.
>> I'm not sure this is safe/correct.  ISTM that you need to look at the
>> underlying TREE_TYPE to check for const-volatile rather than
>> TREE_SIDE_EFFECTS.
>
> Just to quote tree.h:
>
> /* In any expression, decl, or constant, nonzero means it has side effects or
>    reevaluation of the whole expression could produce a different value.
>    This is set if any subexpression is a function call, a side effect or a
>    reference to a volatile variable.  In a ..._DECL, this is set only if the
>    declaration said `volatile'.  This will never be set for a constant.  */
> #define TREE_SIDE_EFFECTS(NODE) \
>   (NON_TYPE_CHECK (NODE)->base.side_effects_flag)
>
> so if exp is a decl then that's the volatile check.
>

Thank you Richard for the review.
Jeff: Can you please let me know if Richard comments reply to your
concerns?

Cupertino

>> Of secondary importance is the ChangeLog.  Just saying "Changed
>> function" provides no real information.  Something like this would be
>> better:
>>
>>         * config/v850/v850.c (v850_select_section): Put const volatile
>>         objects into read-only sections.
>>
>>
>> Jeff
>>
>>
>>
>>
>> > ---
>> >   gcc/config/v850/v850.cc | 1 -
>> >   1 file changed, 1 deletion(-)
>> >
>> > diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
>> > index c7d432990ab..e66893fede4 100644
>> > --- a/gcc/config/v850/v850.cc
>> > +++ b/gcc/config/v850/v850.cc
>> > @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>> >       {
>> >         int is_const;
>> >         if (!TREE_READONLY (exp)
>> > -       || TREE_SIDE_EFFECTS (exp)
>> >         || !DECL_INITIAL (exp)
>> >         || (DECL_INITIAL (exp) != error_mark_node
>> >             && !TREE_CONSTANT (DECL_INITIAL (exp))))
Cupertino Miranda Jan. 19, 2023, 9:59 a.m. UTC | #8 
Hi Jeff,

Kindly calling your attention to this thread.

Regards,
Cupertino

Cupertino Miranda via Gcc-patches writes:

> Richard Biener writes:
>
>> On Mon, Dec 5, 2022 at 7:07 PM Jeff Law via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>>
>>>
>>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>> > Changed target code to select .rodata section for 'const volatile'
>>> > defined variables.
>>> > This change is in the context of the bugzilla #170181.
>>> >
>>> > gcc/ChangeLog:
>>> >
>>> >       v850.c(v850_select_section): Changed function.
>>> I'm not sure this is safe/correct.  ISTM that you need to look at the
>>> underlying TREE_TYPE to check for const-volatile rather than
>>> TREE_SIDE_EFFECTS.
>>
>> Just to quote tree.h:
>>
>> /* In any expression, decl, or constant, nonzero means it has side effects or
>>    reevaluation of the whole expression could produce a different value.
>>    This is set if any subexpression is a function call, a side effect or a
>>    reference to a volatile variable.  In a ..._DECL, this is set only if the
>>    declaration said `volatile'.  This will never be set for a constant.  */
>> #define TREE_SIDE_EFFECTS(NODE) \
>>   (NON_TYPE_CHECK (NODE)->base.side_effects_flag)
>>
>> so if exp is a decl then that's the volatile check.
>>
>
> Thank you Richard for the review.
> Jeff: Can you please let me know if Richard comments reply to your
> concerns?
>
> Cupertino
>
>>> Of secondary importance is the ChangeLog.  Just saying "Changed
>>> function" provides no real information.  Something like this would be
>>> better:
>>>
>>>         * config/v850/v850.c (v850_select_section): Put const volatile
>>>         objects into read-only sections.
>>>
>>>
>>> Jeff
>>>
>>>
>>>
>>>
>>> > ---
>>> >   gcc/config/v850/v850.cc | 1 -
>>> >   1 file changed, 1 deletion(-)
>>> >
>>> > diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
>>> > index c7d432990ab..e66893fede4 100644
>>> > --- a/gcc/config/v850/v850.cc
>>> > +++ b/gcc/config/v850/v850.cc
>>> > @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>>> >       {
>>> >         int is_const;
>>> >         if (!TREE_READONLY (exp)
>>> > -       || TREE_SIDE_EFFECTS (exp)
>>> >         || !DECL_INITIAL (exp)
>>> >         || (DECL_INITIAL (exp) != error_mark_node
>>> >             && !TREE_CONSTANT (DECL_INITIAL (exp))))
Jeff Law Jan. 22, 2023, 6:43 p.m. UTC | #9 
On 1/19/23 02:59, Cupertino Miranda wrote:
> 
> Hi Jeff,
> 
> Kindly calling your attention to this thread.
Sorry, just crazy busy around here.

Jeff
Jeff Law Jan. 22, 2023, 6:49 p.m. UTC | #10 
On 1/9/23 00:57, Richard Biener wrote:
> On Mon, Dec 5, 2022 at 7:07 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>>
>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>> Changed target code to select .rodata section for 'const volatile'
>>> defined variables.
>>> This change is in the context of the bugzilla #170181.
>>>
>>> gcc/ChangeLog:
>>>
>>>        v850.c(v850_select_section): Changed function.
>> I'm not sure this is safe/correct.  ISTM that you need to look at the
>> underlying TREE_TYPE to check for const-volatile rather than
>> TREE_SIDE_EFFECTS.
> 
> Just to quote tree.h:
> 
> /* In any expression, decl, or constant, nonzero means it has side effects or
>     reevaluation of the whole expression could produce a different value.
>     This is set if any subexpression is a function call, a side effect or a
>     reference to a volatile variable.  In a ..._DECL, this is set only if the
>     declaration said `volatile'.  This will never be set for a constant.  */
> #define TREE_SIDE_EFFECTS(NODE) \
>    (NON_TYPE_CHECK (NODE)->base.side_effects_flag)
> 
> so if exp is a decl then that's the volatile check.
Ah, then we can just use TREE_SIDE_EFFECTS for testing if a _DECL node 
is volatile.  So that concern is a non-issue.  I think that was the only 
concern with patch #1.  I'll install it momentarily.

Jeff
diff mbox series

Patch

diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
index c7d432990ab..e66893fede4 100644
--- a/gcc/config/v850/v850.cc
+++ b/gcc/config/v850/v850.cc
@@ -2865,7 +2865,6 @@  v850_select_section (tree exp,
     {
       int is_const;
       if (!TREE_READONLY (exp)
-	  || TREE_SIDE_EFFECTS (exp)
 	  || !DECL_INITIAL (exp)
 	  || (DECL_INITIAL (exp) != error_mark_node
 	      && !TREE_CONSTANT (DECL_INITIAL (exp))))