attribute data structure rewrite

attribute data structure rewrite

[Geoffrey Keating]

[-- Attachment #1: Type: text/plain, Size: 4178 bytes --]

As a precondition for other compile speed improvements, I've been 
rewriting the way that GCC represents attributes.  The result is a 
medium-size patch that touches every language and many targets (a 
surprising number of targets don't define any special attributes at 
all).

The basic principle is that DECL_ATTRIBUTES and TREE_ATTRIBUTES now 
point to an attribute_list:

/* A structure representing 'attributes' on a DECL or TYPE node.
    Each attribute has a NAME (an IDENTIFIER_NODE) and possibly a VALUE. 
  */
struct one_attribute GTY(())
{
   tree name;
   tree value;
};

/* A counted list of attributes.  */
struct attribute_list_s GTY(())
{
   attribute_count n_attributes;
   /* There are 16 bits free here.  */
   struct one_attribute GTY((length ("%h.n_attributes"))) attribs[1];
};

This has many benefits, mostly flowing from the fact that it's not a 
TREE_LIST any more.

1. Do we think this would be acceptable for stage 3, or should I make a 
branch?
2. Would anyone be interested in helping me test it, especially on the 
more difficult architectures like alpha-vms or i386-netware?

diffstat on my current patch looks like:

  ada/gigi.h                 |    2
  ada/utils.c                |    2
  attribs.c                  |  347 
++++++++++++++++++++++++++-------------------
  builtin-attrs.def          |   62 +++-----
  c-common.c                 |  116 +++++++++------
  c-common.h                 |    4
  c-decl.c                   |   76 +++++----
  c-format.c                 |   25 +--
  c-objc-common.c            |    4
  c-parse.in                 |   98 ++++++------
  c-pragma.c                 |   10 -
  c-tree.h                   |   28 +--
  c-typeck.c                 |   12 -
  cgraphunit.c               |    2
  config/alpha/alpha.c       |    6
  config/arm/arm.c           |   37 ++--
  config/arm/pe.c            |   12 -
  config/avr/avr.c           |   14 -
  config/c4x/c4x.c           |    6
  config/frv/frv.c           |    2
  config/h8300/h8300.c       |   35 ----
  config/i386/i386.c         |   42 ++---
  config/i386/netware.c      |    8 -
  config/i386/winnt.c        |   22 +-
  config/ia64/ia64.c         |   10 -
  config/ip2k/ip2k.c         |    5
  config/m32r/m32r.c         |   10 -
  config/m68hc11/m68hc11.c   |   28 +--
  config/m68k/m68k.c         |    5
  config/mcore/mcore.c       |    6
  config/ns32k/ns32k.c       |    4
  config/rs6000/rs6000.c     |   22 +-
  config/sh/sh.c             |   10 -
  config/sh/symbian.c        |   28 +--
  config/sol2.c              |    8 -
  config/stormy16/stormy16.c |    6
  config/v850/v850.c         |   20 --
  coretypes.h                |    6
  cp/call.c                  |    2
  cp/class.c                 |    2
  cp/cp-tree.h               |   29 ++-
  cp/decl.c                  |   59 +++----
  cp/decl.h                  |    2
  cp/decl2.c                 |   14 -
  cp/friend.c                |    2
  cp/method.c                |    4
  cp/name-lookup.c           |   10 -
  cp/name-lookup.h           |    2
  cp/optimize.c              |    2
  cp/parser.c                |  176 ++++++++++++----------
  cp/pt.c                    |    2
  cp/tree.c                  |    4
  cp/typeck.c                |    8 -
  fold-const.c               |    4
  gengtype-lex.l             |    6
  integrate.c                |    9 -
  java/decl.c                |   46 ++---
  java/java-tree.h           |    2
  java/jcf-reader.c          |   12 -
  langhooks.c                |    4
  langhooks.h                |    2
  objc/objc-act.c            |   90 ++++++-----
  passes.c                   |    5
  print-tree.c               |   23 ++
  system.h                   |    2
  target-def.h               |    4
  target.h                   |   10 -
  targhooks.c                |   10 +
  targhooks.h                |    4
  tree-browser.c             |    9 -
  tree-inline.c              |    6
  tree.c                     |  307 
+++++++++++++++++++++++++--------------
  tree.h                     |   81 ++++++++--
  treelang/treetree.c        |    6
  varasm.c                   |    6
  75 files changed, 1171 insertions, 935 deletions

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2408 bytes --]

Re: attribute data structure rewrite

[Andrew Pinski]

On Sep 23, 2004, at 7:54 PM, Geoffrey Keating wrote:

> As a precondition for other compile speed improvements, I've been 
> rewriting the way that GCC represents attributes.  The result is a 
> medium-size patch that touches every language and many targets (a 
> surprising number of targets don't define any special attributes at 
> all).
>
> The basic principle is that DECL_ATTRIBUTES and TREE_ATTRIBUTES now 
> point to an attribute_list:
>
> /* A structure representing 'attributes' on a DECL or TYPE node.
>    Each attribute has a NAME (an IDENTIFIER_NODE) and possibly a 
> VALUE.  */
> struct one_attribute GTY(())
> {
>   tree name;
>   tree value;
> };

Shouldn't name be redefined as char* as you don't really need a full 
IDENTIFIER_NODE?
Yes I know that correctly it is not but I would suspect that it would 
give a good
speedup as you don't need to allocate as many trees as before.


Thanks,
Andrew Pinski

Re: attribute data structure rewrite

[Joseph S. Myers]

On Thu, 23 Sep 2004, Andrew Pinski wrote:

> Shouldn't name be redefined as char* as you don't really need a full 
> IDENTIFIER_NODE? Yes I know that correctly it is not but I would suspect 
> that it would give a good speedup as you don't need to allocate as many 
> trees as before.

The IDENTIFIER_NODEs would probably already have been allocated in the 
course of parsing.  Indeed plain strings would probably be better 
datastructures - and the cleanup suggested in the comment above 
is_attribute_p, keeping the lists in canonical form, might make sense too 
- but better to separate such changes into separate patches from the 
minimal patch that changes the datastructure to the form indicated.

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
  http://www.srcf.ucam.org/~jsm28/gcc/#c90status - status of C90 for GCC 4.0
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Re: attribute data structure rewrite

[Zack Weinberg]

Andrew Pinski <pinskia@physics.uc.edu> writes:

> Shouldn't name be redefined as char* as you don't really need a full 
> IDENTIFIER_NODE?

Or better still, as code numbers (with an identifier->number mapping
through the identifier hash table - as Joseph correctly points out,
the identifiers get allocated during parsing anyway).  Currently we
have to do string comparison to decide which attributes are which
(since any attribute can be written either as "name" or as "__name__").

zw

Re: attribute data structure rewrite

[Geoffrey Keating]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]


On 23/09/2004, at 5:05 PM, Andrew Pinski wrote:

>
> On Sep 23, 2004, at 7:54 PM, Geoffrey Keating wrote:
>
>> As a precondition for other compile speed improvements, I've been 
>> rewriting the way that GCC represents attributes.  The result is a 
>> medium-size patch that touches every language and many targets (a 
>> surprising number of targets don't define any special attributes at 
>> all).
>>
>> The basic principle is that DECL_ATTRIBUTES and TREE_ATTRIBUTES now 
>> point to an attribute_list:
>>
>> /* A structure representing 'attributes' on a DECL or TYPE node.
>>    Each attribute has a NAME (an IDENTIFIER_NODE) and possibly a 
>> VALUE.  */
>> struct one_attribute GTY(())
>> {
>>   tree name;
>>   tree value;
>> };
>
> Shouldn't name be redefined as char* as you don't really need a full 
> IDENTIFIER_NODE?
> Yes I know that correctly it is not but I would suspect that it would 
> give a good
> speedup as you don't need to allocate as many trees as before.

I thought of that, but I suspect it's better to go the other way: make 
'name' always be the *same* IDENTIFIER_NODE for a particular kind of 
attribute, so you can just compare pointers.  You do always have to 
allocate one, because 'name' will come from a token in user code.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2408 bytes --]

Re: attribute data structure rewrite

[Gabriel Dos Reis]

Geoffrey Keating <geoffk@apple.com> writes:

| As a precondition for other compile speed improvements, I've been
| rewriting the way that GCC represents attributes.  The result is a
| medium-size patch that touches every language and many targets (a
| surprising number of targets don't define any special attributes at
| all).
| 
| The basic principle is that DECL_ATTRIBUTES and TREE_ATTRIBUTES now
| point to an attribute_list:
| 
| /* A structure representing 'attributes' on a DECL or TYPE node.
|     Each attribute has a NAME (an IDENTIFIER_NODE) and possibly a
| VALUE. */
| struct one_attribute GTY(())
| {
|    tree name;
|    tree value;
| };
| 
| /* A counted list of attributes.  */
| struct attribute_list_s GTY(())
| {
|    attribute_count n_attributes;
|    /* There are 16 bits free here.  */
|    struct one_attribute GTY((length ("%h.n_attributes"))) attribs[1];
| };
| 
| This has many benefits, mostly flowing from the fact that it's not a
| TREE_LIST any more.
| 
| 1. Do we think this would be acceptable for stage 3, or should I make
| a branch?


I would argue that such a change should be accepted, at this phase of
stage 3; now.  As:
  (1) it addresses compile-time/memory regression;
  (2) it is no less interesting no less useful that the various cleanup
      we've been having, in particular in the C++ front-end.


I would even take a further step and ask an official position about
how we how attributes to play with language rules.


Currently, the compiler fails on any of the following fails,
complaining about  non-"integral constant expression"-ness, which is
pure nonense. 

    const int N = 4;
    struct S {
      enum { M = 4 };
      float x __attibute__((__aligned__(N)));
      float x __attibute__((__aligned__(M)));
    };

   template<typename T, int N>
      struct buffer {
         typedef char U[sizeof(T)];
         U data __attribute__((__aligned__(N)));
     };

They should be accepted.

-- Gaby

Re: attribute data structure rewrite

[Giovanni Bajo]

Gabriel Dos Reis wrote:

> Currently, the compiler fails on any of the following fails,
> complaining about  non-"integral constant expression"-ness, which is
> pure nonense.
> 
>     const int N = 4;
>     struct S {
>       enum { M = 4 };
>       float x __attibute__((__aligned__(N)));
>       float x __attibute__((__aligned__(M)));
>     };
> 
>    template<typename T, int N>
>       struct buffer {
>          typedef char U[sizeof(T)];
>          U data __attribute__((__aligned__(N)));
>      };

Is there already a bug report about this?

Anothing thing I have been shown today is that we currently reject this:

----------------------------------------
#include <stddef.h>
#include <stdio.h>

struct A { char foo[10]; };

void bar(void) {
    int i;
    for (i=0;i<10;i++)
        printf("%d\n", offsetof(struct A, foo[i]));
}
--------------------------------------

but we used to accept it.  Any idea about its legality? Workarounds?

Giovanni Bajo

Re: attribute data structure rewrite

[Gabriel Dos Reis]

"Giovanni Bajo" <giovannibajo@libero.it> writes:

| Gabriel Dos Reis wrote:
| 
| > Currently, the compiler fails on any of the following fails,
| > complaining about  non-"integral constant expression"-ness, which is
| > pure nonense.
| > 
| >     const int N = 4;
| >     struct S {
| >       enum { M = 4 };
| >       float x __attibute__((__aligned__(N)));
| >       float x __attibute__((__aligned__(M)));
| >     };
| > 
| >    template<typename T, int N>
| >       struct buffer {
| >          typedef char U[sizeof(T)];
| >          U data __attribute__((__aligned__(N)));
| >      };
| 
| Is there already a bug report about this?

Hmm, now that you're questioning I think we don't have a proper PR for
them. 

| Anothing thing I have been shown today is that we currently reject this:
| 
| ----------------------------------------
| #include <stddef.h>
| #include <stdio.h>
| 
| struct A { char foo[10]; };
| 
| void bar(void) {
|     int i;
|     for (i=0;i<10;i++)
|         printf("%d\n", offsetof(struct A, foo[i]));
| }
| --------------------------------------
| 
| but we used to accept it.  Any idea about its legality? Workarounds?

My gut reaction is that "foo[i]" is not a data at compile-time
location; so, it does not qualify for use in offsetof.

We used to accept it because we did not implement offsetof correctly.
For example, the following variation should be rejected (in C89 an C++
mode)

   struct A { char foo[10]; };
   void bar(void) {
     int i;
     for (int i = 0; i < 10; ++i)
        {
           char ary[offsetof(struct A, foo[i] + 1);
        }
   }

-- Gaby

Re: attribute data structure rewrite

[Giovanni Bajo]

Gabriel Dos Reis wrote:

> We used to accept it because we did not implement offsetof correctly.
> For example, the following variation should be rejected (in C89 an C++
> mode)
>
>    struct A { char foo[10]; };
>    void bar(void) {
>      int i;
>      for (int i = 0; i < 10; ++i)
>         {
>            char ary[offsetof(struct A, foo[i] + 1);
>         }
>    }

I don't understand your example because I don't think the +1 there is valid.
Unless you meant:

char ary[offsetof(struct A, foo[i])+1];

Anyway, what about this in C99? We don't need a constant expression in this
context, and even if offsetof is always a constant expression we could still
accept it as an extension. In other words, we could transform it to
offsetof(struct A, foo)+i*sizeof(foo[0]) in contexts that don't require a
constant expression.

Giovanni Bajo

Re: attribute data structure rewrite

[Gabriel Dos Reis]

"Giovanni Bajo" <giovannibajo@libero.it> writes:

| Gabriel Dos Reis wrote:
| 
| > We used to accept it because we did not implement offsetof correctly.
| > For example, the following variation should be rejected (in C89 an C++
| > mode)
| >
| >    struct A { char foo[10]; };
| >    void bar(void) {
| >      int i;
| >      for (int i = 0; i < 10; ++i)
| >         {
| >            char ary[offsetof(struct A, foo[i] + 1);
| >         }
| >    }
| 
| I don't understand your example because I don't think the +1 there is valid.
| Unless you meant:
| 
| char ary[offsetof(struct A, foo[i])+1];

Yes, this is what I meant.  
Sorry for the typo, and sending a testcase without compiling :-)

| Anyway, what about this in C99? 

Even in strict C99 mode, it will not be valid.  One could think of GNU
extensions that compute offsetof at run-time, but that is something
we've debated much in the past.  I see the move to the new
implementation as a desire to keep away from that.  
RTH might want to weight in.

| We don't need a constant expression in this
| context, and even if offsetof is always a constant expression we could still
| accept it as an extension. 

I agree that in C99, one does not need a constant expression to make
it work, but the offsetof-subexpression still will not be valid.

| In other words, we could transform it to
| offsetof(struct A, foo)+i*sizeof(foo[0]) in contexts that don't require a
| constant expression.

That is a possibility.  I'm not sure how serious the issue is.

-- Gaby

Re: attribute data structure rewrite

[Andreas Schwab]

"Giovanni Bajo" <giovannibajo@libero.it> writes:

> Anothing thing I have been shown today is that we currently reject this:
>
> ----------------------------------------
> #include <stddef.h>
> #include <stdio.h>
>
> struct A { char foo[10]; };
>
> void bar(void) {
>     int i;
>     for (i=0;i<10;i++)
>         printf("%d\n", offsetof(struct A, foo[i]));
> }
> --------------------------------------
>
> but we used to accept it.  Any idea about its legality? Workarounds?

offsetof (struct A, foo[0]) + i * sizeof (foo[0])

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: attribute data structure rewrite

[Andreas Schwab]

Andreas Schwab <schwab@suse.de> writes:

> "Giovanni Bajo" <giovannibajo@libero.it> writes:
>
>> Anothing thing I have been shown today is that we currently reject this:
>>
>> ----------------------------------------
>> #include <stddef.h>
>> #include <stdio.h>
>>
>> struct A { char foo[10]; };
>>
>> void bar(void) {
>>     int i;
>>     for (i=0;i<10;i++)
>>         printf("%d\n", offsetof(struct A, foo[i]));
>> }
>> --------------------------------------
>>
>> but we used to accept it.  Any idea about its legality? Workarounds?
>
> offsetof (struct A, foo[0]) + i * sizeof (foo[0])

Of course, that doesn't compile either, but you get the idea. :-)

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: attribute data structure rewrite

[Joseph S. Myers]

On Fri, 24 Sep 2004, Giovanni Bajo wrote:

> Anothing thing I have been shown today is that we currently reject this:
> 
> ----------------------------------------
> #include <stddef.h>
> #include <stdio.h>
> 
> struct A { char foo[10]; };
> 
> void bar(void) {
>     int i;
>     for (i=0;i<10;i++)
>         printf("%d\n", offsetof(struct A, foo[i]));
> }
> --------------------------------------
> 
> but we used to accept it.  Any idea about its legality? Workarounds?

Compile-time undefined for both C and C++, no diagnostic required; one 
couldn't reasonably be given with the old macro implementations.  Either 
diagnose the use of non-integer-constant-expressions in offsetof, or 
permit them but make them cause the result not to be an integer constant 
expression.

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
  http://www.srcf.ucam.org/~jsm28/gcc/#c90status - status of C90 for GCC 4.0
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Re: attribute data structure rewrite

[Giovanni Bajo]

Joseph S. Myers wrote:


>> ----------------------------------------
>> #include <stddef.h>
>> #include <stdio.h>
>>
>> struct A { char foo[10]; };
>>
>> void bar(void) {
>>     int i;
>>     for (i=0;i<10;i++)
>>         printf("%d\n", offsetof(struct A, foo[i]));
>> }
>> --------------------------------------
>>
>> but we used to accept it.  Any idea about its legality? Workarounds?
>
> Compile-time undefined for both C and C++, no diagnostic required; one
> couldn't reasonably be given with the old macro implementations.

Yes. Actually, the old implementation just worked.

> Either diagnose the use of non-integer-constant-expressions in
> offsetof, or permit them but make them cause the result not to be an
> integer constant expression.


I think we currently go with the former. But I believe this to be a regression
in user experience. Using the external + sizeof(i)*blah is ugly. If it was
possible to build the result not being an integer constant expression before
4.0 it would be great. If you want, I can file a bug report to track this.

-- 
Giovanni Bajo

Re: attribute data structure rewrite

[Geoffrey Keating]

[-- Attachment #1: Type: text/plain, Size: 2579 bytes --]


On 23/09/2004, at 5:47 PM, Gabriel Dos Reis wrote:

> Geoffrey Keating <geoffk@apple.com> writes:
>
> | As a precondition for other compile speed improvements, I've been
> | rewriting the way that GCC represents attributes.  The result is a
> | medium-size patch that touches every language and many targets (a
> | surprising number of targets don't define any special attributes at
> | all).
> |
> | The basic principle is that DECL_ATTRIBUTES and TREE_ATTRIBUTES now
> | point to an attribute_list:
> |
> | /* A structure representing 'attributes' on a DECL or TYPE node.
> |     Each attribute has a NAME (an IDENTIFIER_NODE) and possibly a
> | VALUE. */
> | struct one_attribute GTY(())
> | {
> |    tree name;
> |    tree value;
> | };
> |
> | /* A counted list of attributes.  */
> | struct attribute_list_s GTY(())
> | {
> |    attribute_count n_attributes;
> |    /* There are 16 bits free here.  */
> |    struct one_attribute GTY((length ("%h.n_attributes"))) attribs[1];
> | };
> |
> | This has many benefits, mostly flowing from the fact that it's not a
> | TREE_LIST any more.
> |
> | 1. Do we think this would be acceptable for stage 3, or should I make
> | a branch?
>
>
> I would argue that such a change should be accepted, at this phase of
> stage 3; now.  As:
>   (1) it addresses compile-time/memory regression;
>   (2) it is no less interesting no less useful that the various cleanup
>       we've been having, in particular in the C++ front-end.
>
>
> I would even take a further step and ask an official position about
> how we how attributes to play with language rules.
>
>
> Currently, the compiler fails on any of the following fails,
> complaining about  non-"integral constant expression"-ness, which is
> pure nonense.
>
>     const int N = 4;
>     struct S {
>       enum { M = 4 };
>       float x __attibute__((__aligned__(N)));
>       float x __attibute__((__aligned__(M)));
>     };
>
>    template<typename T, int N>
>       struct buffer {
>          typedef char U[sizeof(T)];
>          U data __attribute__((__aligned__(N)));
>      };
>
> They should be accepted.

That's a very good point.  I noticed from looking at the C front-end 
that an attribute can have as a 'value' any of:

1. NULL
2. an IDENTIFIER
3. an expression

 From that, I suspect that what's happening here is that 'N' is an 
identifier, so that rule matches in preference to treating 'N' as an 
expression and evaluating it.

I would really like to avoid case (2), or at least make sure that you 
can tell which of (2) or (3) any particular attribute should have.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2408 bytes --]

Re: attribute data structure rewrite

[Joseph S. Myers]

On Thu, 23 Sep 2004, Geoffrey Keating wrote:

> That's a very good point.  I noticed from looking at the C front-end that an
> attribute can have as a 'value' any of:
> 
> 1. NULL
> 2. an IDENTIFIER
> 3. an expression
> 
> From that, I suspect that what's happening here is that 'N' is an identifier,
> so that rule matches in preference to treating 'N' as an expression and
> evaluating it.
> 
> I would really like to avoid case (2), or at least make sure that you can tell
> which of (2) or (3) any particular attribute should have.

That's one of the shift-reduce conflicts in the C parser, between the 
alternatives

attrib:
    /* empty */
                { $$ = NULL_TREE; }
        | any_word
                { $$ = build_tree_list ($1, NULL_TREE); }
        | any_word '(' IDENTIFIER ')'
                { $$ = build_tree_list ($1, build_tree_list (NULL_TREE, $3)); }
        | any_word '(' IDENTIFIER ',' nonnull_exprlist ')'
                { $$ = build_tree_list ($1, tree_cons (NULL_TREE, $3, $5)); }
        | any_word '(' exprlist ')'
                { $$ = build_tree_list ($1, $3); }
        ;

where an identifier might be parsed either way.  The enum case means that 
looking up the identifier to get an expression is needed for C as well as 
C++ (though of course independent from changes to datastructures).

Empty attributes are no trouble.  Many attributes have no arguments.  The 
identifier case applies (for example) to "mode" attributes, "cleanup" 
attributes (where it names a function) and some target-specific 
attributes.  The identifier followed by expression list case applies to 
format attributes.  The expression list case applies to many attributes, 
most taking one argument, some such as "nonnull" taking multiple 
arguments.  It should be well-defined for each attribute what sort of 
arguments it takes.

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
  http://www.srcf.ucam.org/~jsm28/gcc/#c90status - status of C90 for GCC 4.0
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Re: attribute data structure rewrite

[Giovanni Bajo]

Geoffrey Keating wrote:

> /* A structure representing 'attributes' on a DECL or TYPE node.
>     Each attribute has a NAME (an IDENTIFIER_NODE) and possibly a
>   VALUE. */
> struct one_attribute GTY(())
> {
>    tree name;
>    tree value;
> };
> 
> /* A counted list of attributes.  */
> struct attribute_list_s GTY(())
> {
>    attribute_count n_attributes;
>    /* There are 16 bits free here.  */
>    struct one_attribute GTY((length ("%h.n_attributes"))) attribs[1];
> };

Why not simply VEC(one_attribute) ?

Do you have some numbers on the speedup we get with this?

Giovanni Bajo

Re: attribute data structure rewrite

[Geoffrey Keating]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]


On 23/09/2004, at 7:30 PM, Giovanni Bajo wrote:

> Geoffrey Keating wrote:
>
>> /* A structure representing 'attributes' on a DECL or TYPE node.
>>     Each attribute has a NAME (an IDENTIFIER_NODE) and possibly a
>>   VALUE. */
>> struct one_attribute GTY(())
>> {
>>    tree name;
>>    tree value;
>> };
>>
>> /* A counted list of attributes.  */
>> struct attribute_list_s GTY(())
>> {
>>    attribute_count n_attributes;
>>    /* There are 16 bits free here.  */
>>    struct one_attribute GTY((length ("%h.n_attributes"))) attribs[1];
>> };
>
> Why not simply VEC(one_attribute) ?

VECs are much too much for what's needed here.  They're expandable, 
they can be used as a stack, all kinds of stuff (and all kinds of data) 
that aren't necessary.

> Do you have some numbers on the speedup we get with this?

This is infrastructure for speedup patches, not a speedup patch itself. 
  In the compilations I time, only a very small proportion of functions 
and types have any attributes, and most of those are builtin functions.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2408 bytes --]

Re: attribute data structure rewrite

[Nathan Sidwell]

Geoffrey Keating wrote:
> 
> On 23/09/2004, at 7:30 PM, Giovanni Bajo wrote:

>> Why not simply VEC(one_attribute) ?
> 
> 
> VECs are much too much for what's needed here.  They're expandable, they 
> can be used as a stack, all kinds of stuff (and all kinds of data) that 
> aren't necessary.
The extra data they have over your implementation is an 'allocated' field.
And the alloc and num fields are ints, rather than shorts.

It would not be difficult to have additional defining macros which specified
'this never grows to more that 16,000 entries'.  If that was done,
it would become the exact same size as your data structure, and you'd
get the same API as other vectors, rather than a new bespoke API for your
need.

I'd much prefer the same API for all vector-like things.

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: attribute data structure rewrite

[Mark Mitchell]

Geoffrey Keating wrote:

> As a precondition for other compile speed improvements, I've been 
> rewriting the way that GCC represents attributes.  The result is a 
> medium-size patch that touches every language and many targets (a 
> surprising number of targets don't define any special attributes at all).
>
> 1. Do we think this would be acceptable for stage 3, or should I make 
> a branch?

It might be acceptable for Stage 3 if the subsequent follow-on patches 
demonstrated a noticable performance improvement.  It should be a branch 
until you can demonstrate that improvement.  The final decision on 
acceptability will probably hinge on how mechanical the patch is: even 
if large, if it is quite mechanical (as I would anticipate), that will 
make it less worrisome.

-- 
Mark Mitchell
CodeSourcery, LLC
(916) 791-8304
mark@codesourcery.com