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

2023-01-22 Thread Jeff Law via Gcc-patches 




On 1/9/23 00:57, Richard Biener wrote:

On Mon, Dec 5, 2022 at 7:07 PM Jeff Law via Gcc-patches
 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

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

2023-01-22 Thread Jeff Law via Gcc-patches 




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

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

2023-01-19 Thread Cupertino Miranda via Gcc-patches 


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

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

2023-01-13 Thread Cupertino Miranda via Gcc-patches 


Richard Biener writes:

> On Mon, Dec 5, 2022 at 7:07 PM Jeff Law via Gcc-patches
>  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

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

2023-01-08 Thread Richard Biener via Gcc-patches 
On Mon, Dec 5, 2022 at 7:07 PM Jeff Law via Gcc-patches
 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

Re: [PING] Re: [PATCH 1/2] select .rodata for const volatile variables.

2023-01-02 Thread Cupertino Miranda via Gcc-patches 


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

[PING] Re: [PATCH 1/2] select .rodata for const volatile variables.

2022-12-22 Thread Cupertino Miranda via Gcc-patches 


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

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

2022-12-15 Thread Cupertino Miranda via Gcc-patches 


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

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

2022-12-07 Thread Cupertino Miranda via Gcc-patches 


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

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

2022-12-05 Thread Jeff Law via Gcc-patches 




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

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

2022-12-02 Thread Cupertino Miranda via Gcc-patches 
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(-)

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
-- 
2.30.2