Where in C++ module streaming to handle a new bitfield added in "tree_decl_common"

Where in C++ module streaming to handle a new bitfield added in "tree_decl_common"

[Qing Zhao]

Hi, Nathan,

I am adding a new bitfield “decl_not_flexarray” in “tree_decl_common”  (gcc/tree-core.h) for the new gcc feature -fstrict-flex-arrays. 

====
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index ea9f281f1cc..458c6e6ceea 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -1813,7 +1813,10 @@ struct GTY(()) tree_decl_common {
     TYPE_WARN_IF_NOT_ALIGN.  */
  unsigned int warn_if_not_align : 6;

-  /* 14 bits unused.  */
+  /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY.  */
+  unsigned int decl_not_flexarray : 1;
+
+  /* 13 bits unused.  */

  /* UID for points-to sets, stable over copying from inlining.  */
  unsigned int pt_uid;
====

(Please refer to the following for details:

https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598556.html
https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598965.html
)

Richard mentioned the following:

"I've not seen it so you are probably missing it - the bit has to be
streamed in tree-streamer-{in,out}.cc to be usable from LTO.  Possibly
C++ module streaming also needs to handle it.”

I have figured out that where to add the handling of the bit in “tree-streamer-{in, out}.cc,
However, it’s quite difficult for me to locate where should I add the handling of this new bit in 
C++ module streaming,  could you please help me on this?

Thanks a lot for your help.

Qing

Re: Where in C++ module streaming to handle a new bitfield added in "tree_decl_common"

[Nathan Sidwell]

On 8/2/22 10:44, Qing Zhao wrote:
> Hi, Nathan,
> 
> I am adding a new bitfield “decl_not_flexarray” in “tree_decl_common”  (gcc/tree-core.h) for the new gcc feature -fstrict-flex-arrays.
> 
> ====
> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> index ea9f281f1cc..458c6e6ceea 100644
> --- a/gcc/tree-core.h
> +++ b/gcc/tree-core.h
> @@ -1813,7 +1813,10 @@ struct GTY(()) tree_decl_common {
>       TYPE_WARN_IF_NOT_ALIGN.  */
>    unsigned int warn_if_not_align : 6;
> 
> -  /* 14 bits unused.  */
> +  /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY.  */
> +  unsigned int decl_not_flexarray : 1;

Is it possible to invert the meaning here -- set the flag if it /IS/ a 
flexible array? negated flags can be confusing, and I see your patch 
sets it to '!is_flexible_array (...)' anyway?

> +
> +  /* 13 bits unused.  */
> 
>    /* UID for points-to sets, stable over copying from inlining.  */
>    unsigned int pt_uid;
> ====
> 
> (Please refer to the following for details:
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598556.html
> https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598965.html



> )
> 
> Richard mentioned the following:
> 
> "I've not seen it so you are probably missing it - the bit has to be
> streamed in tree-streamer-{in,out}.cc to be usable from LTO.  Possibly
> C++ module streaming also needs to handle it.”
> 
> I have figured out that where to add the handling of the bit in “tree-streamer-{in, out}.cc,
> However, it’s quite difficult for me to locate where should I add the handling of this new bit in
> C++ module streaming,  could you please help me on this?
> 


add it in to trees_{in,out}::core_bools.  You could elide streaming for 
non-FIELD_DECL decls.

Hope that helps.

nathan



> Thanks a lot for your help.
> 
> Qing


-- 
Nathan Sidwell

Re: Where in C++ module streaming to handle a new bitfield added in "tree_decl_common"

[Richard Biener]

On Mon, Aug 15, 2022 at 3:29 PM Nathan Sidwell via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 8/2/22 10:44, Qing Zhao wrote:
> > Hi, Nathan,
> >
> > I am adding a new bitfield “decl_not_flexarray” in “tree_decl_common”  (gcc/tree-core.h) for the new gcc feature -fstrict-flex-arrays.
> >
> > ====
> > diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> > index ea9f281f1cc..458c6e6ceea 100644
> > --- a/gcc/tree-core.h
> > +++ b/gcc/tree-core.h
> > @@ -1813,7 +1813,10 @@ struct GTY(()) tree_decl_common {
> >       TYPE_WARN_IF_NOT_ALIGN.  */
> >    unsigned int warn_if_not_align : 6;
> >
> > -  /* 14 bits unused.  */
> > +  /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY.  */
> > +  unsigned int decl_not_flexarray : 1;
>
> Is it possible to invert the meaning here -- set the flag if it /IS/ a
> flexible array? negated flags can be confusing, and I see your patch
> sets it to '!is_flexible_array (...)' anyway?

The issue is it's consumed by the middle-end but set by a single (or two)
frontends and the conservative setting is having the bit not set.  That works
nicely together with touching just the frontends that want stricter behavior
than currently ...

> > +
> > +  /* 13 bits unused.  */
> >
> >    /* UID for points-to sets, stable over copying from inlining.  */
> >    unsigned int pt_uid;
> > ====
> >
> > (Please refer to the following for details:
> >
> > https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598556.html
> > https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598965.html
>
>
>
> > )
> >
> > Richard mentioned the following:
> >
> > "I've not seen it so you are probably missing it - the bit has to be
> > streamed in tree-streamer-{in,out}.cc to be usable from LTO.  Possibly
> > C++ module streaming also needs to handle it.”
> >
> > I have figured out that where to add the handling of the bit in “tree-streamer-{in, out}.cc,
> > However, it’s quite difficult for me to locate where should I add the handling of this new bit in
> > C++ module streaming,  could you please help me on this?
> >
>
>
> add it in to trees_{in,out}::core_bools.  You could elide streaming for
> non-FIELD_DECL decls.
>
> Hope that helps.
>
> nathan
>
>
>
> > Thanks a lot for your help.
> >
> > Qing
>
>
> --
> Nathan Sidwell

Re: Where in C++ module streaming to handle a new bitfield added in "tree_decl_common"

[Nathan Sidwell]

On 8/15/22 10:03, Richard Biener wrote:
> On Mon, Aug 15, 2022 at 3:29 PM Nathan Sidwell via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> On 8/2/22 10:44, Qing Zhao wrote:
>>> Hi, Nathan,
>>>
>>> I am adding a new bitfield “decl_not_flexarray” in “tree_decl_common”  (gcc/tree-core.h) for the new gcc feature -fstrict-flex-arrays.
>>>
>>> ====
>>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
>>> index ea9f281f1cc..458c6e6ceea 100644
>>> --- a/gcc/tree-core.h
>>> +++ b/gcc/tree-core.h
>>> @@ -1813,7 +1813,10 @@ struct GTY(()) tree_decl_common {
>>>        TYPE_WARN_IF_NOT_ALIGN.  */
>>>     unsigned int warn_if_not_align : 6;
>>>
>>> -  /* 14 bits unused.  */
>>> +  /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY.  */
>>> +  unsigned int decl_not_flexarray : 1;
>>
>> Is it possible to invert the meaning here -- set the flag if it /IS/ a
>> flexible array? negated flags can be confusing, and I see your patch
>> sets it to '!is_flexible_array (...)' anyway?
> 
> The issue is it's consumed by the middle-end but set by a single (or two)
> frontends and the conservative setting is having the bit not set.  That works
> nicely together with touching just the frontends that want stricter behavior
> than currently ...

Makes sense, but is the comment incomplete?  I'm guessing this flag is 
for FIELD_DECLs /of array type/, and not just any old FIELD_DECL?  After 
all a field of type int is not a flexible array, but presumably doesn't 
need this flag setting?

nathan

-- 
Nathan Sidwell

Re: Where in C++ module streaming to handle a new bitfield added in "tree_decl_common"

[Richard Biener]

On Tue, Aug 16, 2022 at 2:16 PM Nathan Sidwell <nathan@acm.org> wrote:
>
> On 8/15/22 10:03, Richard Biener wrote:
> > On Mon, Aug 15, 2022 at 3:29 PM Nathan Sidwell via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> On 8/2/22 10:44, Qing Zhao wrote:
> >>> Hi, Nathan,
> >>>
> >>> I am adding a new bitfield “decl_not_flexarray” in “tree_decl_common”  (gcc/tree-core.h) for the new gcc feature -fstrict-flex-arrays.
> >>>
> >>> ====
> >>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> >>> index ea9f281f1cc..458c6e6ceea 100644
> >>> --- a/gcc/tree-core.h
> >>> +++ b/gcc/tree-core.h
> >>> @@ -1813,7 +1813,10 @@ struct GTY(()) tree_decl_common {
> >>>        TYPE_WARN_IF_NOT_ALIGN.  */
> >>>     unsigned int warn_if_not_align : 6;
> >>>
> >>> -  /* 14 bits unused.  */
> >>> +  /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY.  */
> >>> +  unsigned int decl_not_flexarray : 1;
> >>
> >> Is it possible to invert the meaning here -- set the flag if it /IS/ a
> >> flexible array? negated flags can be confusing, and I see your patch
> >> sets it to '!is_flexible_array (...)' anyway?
> >
> > The issue is it's consumed by the middle-end but set by a single (or two)
> > frontends and the conservative setting is having the bit not set.  That works
> > nicely together with touching just the frontends that want stricter behavior
> > than currently ...
>
> Makes sense, but is the comment incomplete?  I'm guessing this flag is
> for FIELD_DECLs /of array type/, and not just any old FIELD_DECL?  After
> all a field of type int is not a flexible array, but presumably doesn't
> need this flag setting?

Yes, the docs should be more complete in tree.h on the actual DECL_NOT_FLEXARRAY
definition.

Richard.

> nathan
>
> --
> Nathan Sidwell

Re: Where in C++ module streaming to handle a new bitfield added in "tree_decl_common"

[Qing Zhao]

> On Aug 16, 2022, at 8:37 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Tue, Aug 16, 2022 at 2:16 PM Nathan Sidwell <nathan@acm.org> wrote:
>> 
>> On 8/15/22 10:03, Richard Biener wrote:
>>> On Mon, Aug 15, 2022 at 3:29 PM Nathan Sidwell via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>> 
>>>> On 8/2/22 10:44, Qing Zhao wrote:
>>>>> Hi, Nathan,
>>>>> 
>>>>> I am adding a new bitfield “decl_not_flexarray” in “tree_decl_common”  (gcc/tree-core.h) for the new gcc feature -fstrict-flex-arrays.
>>>>> 
>>>>> ====
>>>>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
>>>>> index ea9f281f1cc..458c6e6ceea 100644
>>>>> --- a/gcc/tree-core.h
>>>>> +++ b/gcc/tree-core.h
>>>>> @@ -1813,7 +1813,10 @@ struct GTY(()) tree_decl_common {
>>>>>       TYPE_WARN_IF_NOT_ALIGN.  */
>>>>>    unsigned int warn_if_not_align : 6;
>>>>> 
>>>>> -  /* 14 bits unused.  */
>>>>> +  /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY.  */
>>>>> +  unsigned int decl_not_flexarray : 1;
>>>> 
>>>> Is it possible to invert the meaning here -- set the flag if it /IS/ a
>>>> flexible array? negated flags can be confusing, and I see your patch
>>>> sets it to '!is_flexible_array (...)' anyway?
>>> 
>>> The issue is it's consumed by the middle-end but set by a single (or two)
>>> frontends and the conservative setting is having the bit not set.  That works
>>> nicely together with touching just the frontends that want stricter behavior
>>> than currently ...
>> 
>> Makes sense, but is the comment incomplete?  I'm guessing this flag is
>> for FIELD_DECLs /of array type/, and not just any old FIELD_DECL?  After
>> all a field of type int is not a flexible array, but presumably doesn't
>> need this flag setting?
> 
> Yes, the docs should be more complete in tree.h on the actual DECL_NOT_FLEXARRAY
> definition.

Okay, will add more comments in tree.h to make the DECL_NOT_FLEXARRAY more complete.

thanks.

Qing
> 
> Richard.
> 
>> nathan
>> 
>> --
>> Nathan Sidwell

Re: Where in C++ module streaming to handle a new bitfield added in "tree_decl_common"

[Qing Zhao]

> On Aug 15, 2022, at 9:28 AM, Nathan Sidwell <nathan@acm.org> wrote:
> 
> On 8/2/22 10:44, Qing Zhao wrote:
>> Hi, Nathan,
>> I am adding a new bitfield “decl_not_flexarray” in “tree_decl_common”  (gcc/tree-core.h) for the new gcc feature -fstrict-flex-arrays.
>> ====
>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
>> index ea9f281f1cc..458c6e6ceea 100644
>> --- a/gcc/tree-core.h
>> +++ b/gcc/tree-core.h
>> @@ -1813,7 +1813,10 @@ struct GTY(()) tree_decl_common {
>>      TYPE_WARN_IF_NOT_ALIGN.  */
>>   unsigned int warn_if_not_align : 6;
>> -  /* 14 bits unused.  */
>> +  /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY.  */
>> +  unsigned int decl_not_flexarray : 1;
> 
> Is it possible to invert the meaning here -- set the flag if it /IS/ a flexible array? negated flags can be confusing, and I see your patch sets it to '!is_flexible_array (...)' anyway?
> 
>> +
>> +  /* 13 bits unused.  */
>>   /* UID for points-to sets, stable over copying from inlining.  */
>>   unsigned int pt_uid;
>> ====
>> (Please refer to the following for details:
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598556.html
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598965.html
> 
> 
> 
>> )
>> Richard mentioned the following:
>> "I've not seen it so you are probably missing it - the bit has to be
>> streamed in tree-streamer-{in,out}.cc to be usable from LTO.  Possibly
>> C++ module streaming also needs to handle it.”
>> I have figured out that where to add the handling of the bit in “tree-streamer-{in, out}.cc,
>> However, it’s quite difficult for me to locate where should I add the handling of this new bit in
>> C++ module streaming,  could you please help me on this?
> 
> 
> add it in to trees_{in,out}::core_bools.  You could elide streaming for non-FIELD_DECL decls.

Got it. Thanks a lot.

Qing
> 
> Hope that helps.
> 
> nathan
> 
> 
> 
>> Thanks a lot for your help.
>> Qing
> 
> 
> -- 
> Nathan Sidwell