[google]: initialize language field for clone function struct

[google]: initialize language field for clone function struct

[Xinliang David Li]

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

During function cloning, the language field of the src func is not
copied. This can lead to null dereference when gcc calls into langhook
functions.  Unfortunately, I lost track of the test case.

Ok for trunk ?

Thanks,

David


2011-04-29  Xinliang David Li  <davidxl@google.com>

	* tree-inline.c (ininitialize_cfun): Initialize
	language field for clone cfun.

[-- Attachment #2: cln_lang.p --]
[-- Type: text/x-pascal, Size: 464 bytes --]

Index: tree-inline.c
===================================================================
--- tree-inline.c	(revision 173176)
+++ tree-inline.c	(working copy)
@@ -2087,6 +2087,7 @@ initialize_cfun (tree new_fndecl, tree c
   cfun->returns_struct = src_cfun->returns_struct;
   cfun->returns_pcc_struct = src_cfun->returns_pcc_struct;
   cfun->after_tree_profile = src_cfun->after_tree_profile;
+  cfun->language = src_cfun->language;
 
   init_empty_tree_cfg ();
 

Re: [google]: initialize language field for clone function struct

[Xinliang David Li]

Is this one ok?

David

On Fri, Apr 29, 2011 at 4:38 PM, Xinliang David Li <davidxl@google.com> wrote:
> During function cloning, the language field of the src func is not
> copied. This can lead to null dereference when gcc calls into langhook
> functions.  Unfortunately, I lost track of the test case.
>
> Ok for trunk ?
>
> Thanks,
>
> David
>
>
> 2011-04-29  Xinliang David Li  <davidxl@google.com>
>
>        * tree-inline.c (ininitialize_cfun): Initialize
>        language field for clone cfun.
>

Re: [google]: initialize language field for clone function struct

[Jan Hubicka]

> Is this one ok?
Hi,
we did quite some work on removing a langhooks for LTO, where they become quite impossible
So I would like to know what testcase causes the problem and why.

Honza
> 
> David
> 
> On Fri, Apr 29, 2011 at 4:38 PM, Xinliang David Li <davidxl@google.com> wrote:
> > During function cloning, the language field of the src func is not
> > copied. This can lead to null dereference when gcc calls into langhook
> > functions.  Unfortunately, I lost track of the test case.
> >
> > Ok for trunk ?
> >
> > Thanks,
> >
> > David
> >
> >
> > 2011-04-29  Xinliang David Li  <davidxl@google.com>
> >
> >        * tree-inline.c (ininitialize_cfun): Initialize
> >        language field for clone cfun.
> >

Re: [google]: initialize language field for clone function struct

[Xinliang David Li]

On Tue, May 3, 2011 at 3:55 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Is this one ok?
> Hi,
> we did quite some work on removing a langhooks for LTO, where they become quite impossible
> So I would like to know what testcase causes the problem and why.

In fold-const.c, there are many of calls to
lang_hooks.decls.global_bindings_p, and the implementation of this in
name-lookup.h will look at the the cfun->language


#define cp_function_chain (cfun->language)

#define current_binding_level			\
  (*(cfun && cp_function_chain && cp_function_chain->bindings \
   ? &cp_function_chain->bindings		\

int
global_bindings_p (void)
{
  return global_scope_p (current_binding_level);
}


In gcc 4.4.3, current_binding_level is defined in a way that
cp_function_chain->bindings is not guarded, resulting in segfault for
clones. In trunk, this is guarded -- but not setting language field
for clone probably just hide the problem.


Thanks,

David


>
> Honza
>>
>> David
>>
>> On Fri, Apr 29, 2011 at 4:38 PM, Xinliang David Li <davidxl@google.com> wrote:
>> > During function cloning, the language field of the src func is not
>> > copied. This can lead to null dereference when gcc calls into langhook
>> > functions.  Unfortunately, I lost track of the test case.
>> >
>> > Ok for trunk ?
>> >
>> > Thanks,
>> >
>> > David
>> >
>> >
>> > 2011-04-29  Xinliang David Li  <davidxl@google.com>
>> >
>> >        * tree-inline.c (ininitialize_cfun): Initialize
>> >        language field for clone cfun.
>> >
>

Re: [google]: initialize language field for clone function struct

[Jan Hubicka]

> In fold-const.c, there are many of calls to
> lang_hooks.decls.global_bindings_p, and the implementation of this in
> name-lookup.h will look at the the cfun->language
> 
> 
> #define cp_function_chain (cfun->language)
> 
> #define current_binding_level			\
>   (*(cfun && cp_function_chain && cp_function_chain->bindings \
>    ? &cp_function_chain->bindings		\
> 
> int
> global_bindings_p (void)
> {
>   return global_scope_p (current_binding_level);
> }
> 
> 
> In gcc 4.4.3, current_binding_level is defined in a way that
> cp_function_chain->bindings is not guarded, resulting in segfault for
> clones. In trunk, this is guarded -- but not setting language field
> for clone probably just hide the problem.

Indeed, it seems to me that global_bindings_p should go, clearly the LTO does
not preserve its behaviour in any sane way.
I am however completely missing the point of this langhook and doc is not exactly
informative either:

  /* Returns nonzero if we are in the global binding level.  Ada
     returns -1 for an undocumented reason used in stor-layout.c.  */

What is the purpose of this hook?

Honza

Re: [google]: initialize language field for clone function struct

[Eric Botcazou]

> I am however completely missing the point of this langhook and doc is not
> exactly informative either:
>
>   /* Returns nonzero if we are in the global binding level.  Ada
>      returns -1 for an undocumented reason used in stor-layout.c.  */
>
> What is the purpose of this hook?

I've seen things far more undocumented in GCC than this one ;-)


tree
variable_size (tree size)
{

[...]

  /* If the language-processor is to take responsibility for variable-sized
     items (e.g., languages which have elaboration procedures like Ada),
     just return SIZE unchanged.  */
  if (lang_hooks.decls.global_bindings_p () < 0)
    return size;

[...]

  if (lang_hooks.decls.global_bindings_p ())
    {
      if (TREE_CONSTANT (size))
	error ("type size can%'t be explicitly evaluated");
      else
	error ("variable-size type declared outside of any function");

      return size_one_node;
    }

[...]

}


In Ada there are variable-sized types at global level so it isn't an error to 
call variable_size when lang_hooks.decls.global_bindings_p returns non-zero.

-- 
Eric Botcazou

Re: [google]: initialize language field for clone function struct

[Joseph S. Myers]

On Tue, 3 May 2011, Eric Botcazou wrote:

> > I am however completely missing the point of this langhook and doc is not
> > exactly informative either:
> >
> >   /* Returns nonzero if we are in the global binding level.  Ada
> >      returns -1 for an undocumented reason used in stor-layout.c.  */
> >
> > What is the purpose of this hook?
> 
> I've seen things far more undocumented in GCC than this one ;-)
> 
> 
> tree
> variable_size (tree size)
> {
> 
> [...]
> 
>   /* If the language-processor is to take responsibility for variable-sized
>      items (e.g., languages which have elaboration procedures like Ada),
>      just return SIZE unchanged.  */
>   if (lang_hooks.decls.global_bindings_p () < 0)
>     return size;

In my view we should require front ends to take responsibility for 
variable-size types, and get rid of this language-independent function 
(copying such parts as are needed into the front ends that need them).  C 
for example uses its own version as the language-independent one isn't 
right for C.  Similarly, the pending_size code should be local to front 
ends.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [google]: initialize language field for clone function struct

[Nathan Froyd]

On Tue, May 03, 2011 at 07:06:58PM +0000, Joseph S. Myers wrote:
> In my view we should require front ends to take responsibility for 
> variable-size types, and get rid of this language-independent function 
> (copying such parts as are needed into the front ends that need them).  C 
> for example uses its own version as the language-independent one isn't 
> right for C.  Similarly, the pending_size code should be local to front 
> ends.

The raft of changes/improvements enabled by this change would be most
welcome.  *_SIZE becoming double_ints or HOST_WIDE_INT instead of trees
is the first thing that comes to mind; perhaps there are others.

-Nathan

Re: [google]: initialize language field for clone function struct

[Joseph S. Myers]

On Tue, 3 May 2011, Nathan Froyd wrote:

> On Tue, May 03, 2011 at 07:06:58PM +0000, Joseph S. Myers wrote:
> > In my view we should require front ends to take responsibility for 
> > variable-size types, and get rid of this language-independent function 
> > (copying such parts as are needed into the front ends that need them).  C 
> > for example uses its own version as the language-independent one isn't 
> > right for C.  Similarly, the pending_size code should be local to front 
> > ends.
> 
> The raft of changes/improvements enabled by this change would be most
> welcome.  *_SIZE becoming double_ints or HOST_WIDE_INT instead of trees
> is the first thing that comes to mind; perhaps there are others.

I don't see how you can do that; you'll still have variable-sized types 
and objects, it would just be entirely the front end's responsibility to 
ensure that the size expression is evaluated exactly once (whenever 
required by the language).  At most, the size would either be a constant 
or an internal DECL created by the front end if you move that to the front 
end instead of using SAVE_EXPR.

However, I think the fields storing type and decl sizes (and alignment) in 
*bits* are suspect.  Values in bits are needed when dealing with bit-field 
layout, but I think types should otherwise be defined to be made up of 
whole bytes (bytes of BITS_PER_UNIT bits, at this level) with code dealing 
with bit-fields looking at TYPE_PRECISION as needed.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [google]: initialize language field for clone function struct

[Nathan Froyd]

On Tue, May 03, 2011 at 07:27:24PM +0000, Joseph S. Myers wrote:
> On Tue, 3 May 2011, Nathan Froyd wrote:
> > The raft of changes/improvements enabled by this change would be most
> > welcome.  *_SIZE becoming double_ints or HOST_WIDE_INT instead of trees
> > is the first thing that comes to mind; perhaps there are others.
> 
> I don't see how you can do that; you'll still have variable-sized types 
> and objects, it would just be entirely the front end's responsibility to 
> ensure that the size expression is evaluated exactly once (whenever 
> required by the language).

Ah, you're right.  I got carried away about thinking about FE-specific
data structures for things.

-Nathan

Re: [google]: initialize language field for clone function struct

[Eric Botcazou]

> In my view we should require front ends to take responsibility for
> variable-size types, and get rid of this language-independent function
> (copying such parts as are needed into the front ends that need them).

I don't really see the point here.  GCC supports variable-sized types in the 
middle-end (stor-layout.c, gimplifier) so why special-casing this function?

> C for example uses its own version as the language-independent one isn't
> right for C.

The C version looks an almost exact duplicate of the generic one though.  We 
should probably try to reconciliate the different versions.

> Similarly, the pending_size code should be local to front ends.

Yes, I agree here.  gigi already does the full management and doesn't use it.

-- 
Eric Botcazou

Re: [google]: initialize language field for clone function struct

[Joseph S. Myers]

On Tue, 3 May 2011, Eric Botcazou wrote:

> > In my view we should require front ends to take responsibility for
> > variable-size types, and get rid of this language-independent function
> > (copying such parts as are needed into the front ends that need them).
> 
> I don't really see the point here.  GCC supports variable-sized types in the 
> middle-end (stor-layout.c, gimplifier) so why special-casing this function?

In general errors from outside the front end, but relating to problems 
with the user's source code, are suspicious; the front end should detect 
invalid code, diagnose it and not pass it to the language-independent 
compiler.  And the exact language semantics regarding where variable-size 
types are permitted and when the sizes should be evaluated are inherently 
front-end-specific and not easily represented in a generic function.

convert.c is another place with errors that I think ought to go in the 
front ends; having some generic conversion logic is fine, but the front 
ends should deal with diagnosing invalid cases and convert.c should ICE if 
asked to do a conversion it doesn't know how to do.

> > C for example uses its own version as the language-independent one isn't
> > right for C.
> 
> The C version looks an almost exact duplicate of the generic one though.  We 
> should probably try to reconciliate the different versions.

C doesn't want the language-independent errors (see above); they don't 
reflect the right logic for C.  It needs to override parts of the generic 
function when called from generic code, and so to replace it when called 
from the front end.

C returns -1 from global_bindings_p, as does Ada.  That the languages that 
probably care most about variable-size types find aspects of the generic 
function need overriding like that should be a good indication that it 
isn't really that generic - as I said above, semantics for variable sizes 
are very front-end-specific.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [google]: initialize language field for clone function struct

[Eric Botcazou]

> C returns -1 from global_bindings_p, as does Ada.  That the languages that
> probably care most about variable-size types find aspects of the generic
> function need overriding like that should be a good indication that it
> isn't really that generic - as I said above, semantics for variable sizes
> are very front-end-specific.

I think that a large chunk of the non-generic code can be removed because it 
is either useless (the error) or obsolete (pending_size) in the 4.x series.
After that, if you prefer to make this particular function FE-specific, fine 
with me, but they will all essentially look alike.

-- 
Eric Botcazou

Re: [google]: initialize language field for clone function struct

[Joseph S. Myers]

On Tue, 3 May 2011, Eric Botcazou wrote:

> > C returns -1 from global_bindings_p, as does Ada.  That the languages that
> > probably care most about variable-size types find aspects of the generic
> > function need overriding like that should be a good indication that it
> > isn't really that generic - as I said above, semantics for variable sizes
> > are very front-end-specific.
> 
> I think that a large chunk of the non-generic code can be removed because it 
> is either useless (the error) or obsolete (pending_size) in the 4.x series.

Well - the errors in variable_size aren't used by C any more (since 4.5), 
but I don't know whether any other languages use them.  And pending sizes 
are used to a limited extent for C (to handle side effects in sizes of 
array parameters, as described in the comment

  /* ??? Insert the contents of the pending sizes list into the function
     to be evaluated.  The only reason left to have this is
        void foo(int n, int array[n++])
     because we throw away the array type in favor of a pointer type, and
     thus won't naturally see the SAVE_EXPR containing the increment.  All
     other pending sizes would be handled by gimplify_parameters.  */

) although it would now be better to make use of the "expr" parameter to 
grokdeclarator to replace this residual use of the pending sizes global 
list.

The most suspect case for using generic variable-size code would be C++, 
which has some limited, poorly-defined VLA support as a GNU extension.  I 
don't know if Fortran, Java or Go use variable-size types at all.

> After that, if you prefer to make this particular function FE-specific, fine 
> with me, but they will all essentially look alike.

Actually, it will probably be possible the eliminate the function 
completely for C; there's no good reason to do more than calling save_expr 
directly.

  if (TREE_CONSTANT (size))
    return size;

Redundant, save_expr checks for constants.

  size = save_expr (size);

The only necessary bit of c_variable_size once pending sizes are replaced 
by a better scheme.

  save = skip_simple_arithmetic (size);

Premature optimization.

  if (cfun && cfun->dont_save_pending_sizes_p)
    return size;

  if (!global_bindings_p ())
    put_pending_size (save);

No longer needed once pending sizes are replaced.

  return size;

So this just becomes equivalent to save_expr.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [google]: initialize language field for clone function struct

[Eric Botcazou]

> Well - the errors in variable_size aren't used by C any more (since 4.5),
> but I don't know whether any other languages use them.

Apparently not, let's remove them.

> And pending sizes are used to a limited extent for C (to handle side effects
> in sizes of array parameters, as described in the comment
>
>   /* ??? Insert the contents of the pending sizes list into the function
>      to be evaluated.  The only reason left to have this is
>         void foo(int n, int array[n++])
>      because we throw away the array type in favor of a pointer type, and
>      thus won't naturally see the SAVE_EXPR containing the increment.  All
>      other pending sizes would be handled by gimplify_parameters.  */
>
> ) although it would now be better to make use of the "expr" parameter to
> grokdeclarator to replace this residual use of the pending sizes global
> list.

c-parser.c/c-decl.c are the only remaining users of pending sizes so it would 
indeed be desirable to change them so as to totally get rid of pending sizes.

> Actually, it will probably be possible the eliminate the function
> completely for C; there's no good reason to do more than calling save_expr
> directly.
>
>   if (TREE_CONSTANT (size))
>     return size;
>
> Redundant, save_expr checks for constants.
>
>   size = save_expr (size);
>
> The only necessary bit of c_variable_size once pending sizes are replaced
> by a better scheme.
>
>   save = skip_simple_arithmetic (size);
>
> Premature optimization.
>
>   if (cfun && cfun->dont_save_pending_sizes_p)
>     return size;
>
>   if (!global_bindings_p ())
>     put_pending_size (save);
>
> No longer needed once pending sizes are replaced.
>
>   return size;
>
> So this just becomes equivalent to save_expr.

There is the CONTAINS_PLACEHOLDER_P check in the generic case but, yes, 
otherwise it's just a proxy for save_expr.

-- 
Eric Botcazou

Re: [google]: initialize language field for clone function struct

[Tom Tromey]

Joseph> I don't know if Fortran, Java or Go use variable-size types at
Joseph> all.

Java doesn't have variable-size types.
AFAIK, gcj does not generate them internally for anything, either.

Tom

Re: [google]: initialize language field for clone function struct

[Richard Guenther]

On Tue, May 3, 2011 at 6:15 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Tue, May 3, 2011 at 3:55 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> Is this one ok?
>> Hi,
>> we did quite some work on removing a langhooks for LTO, where they become quite impossible
>> So I would like to know what testcase causes the problem and why.
>
> In fold-const.c, there are many of calls to
> lang_hooks.decls.global_bindings_p, and the implementation of this in
> name-lookup.h will look at the the cfun->language

Umm, I think most of them (if not all) are just bogus.  If a FE doesn't
want to fold some stuff when at global scope it should not call fold.
But I'm not sure that is actually what it tries to do ... and the existing
checks are far from consistently spread out in fold-const.c ...

Richard.

Re: [google]: initialize language field for clone function struct

[Eric Botcazou]

> Umm, I think most of them (if not all) are just bogus.  If a FE doesn't
> want to fold some stuff when at global scope it should not call fold.

That isn't so easy because fold is invoked on sizes of types by stor-layout.c 
and these sizes can be variable (at least in Ada).  So I think that the calls 
to the hook are still needed.  But:
  1) The -1 thing should go and the hook return boolean.  The prerequisite is 
to tidy up variable_size,
  2) I think that the GIMPLE hook can return 0 unconditionally.

> But I'm not sure that is actually what it tries to do ... and the existing
> checks are far from consistently spread out in fold-const.c ...

It prevents save_expr from being called at global level, since you cannot 
create SAVE_EXPRs outside functions.  Likewise in variable_size.

-- 
Eric Botcazou

Re: [google]: initialize language field for clone function struct

[Joseph S. Myers]

On Wed, 4 May 2011, Eric Botcazou wrote:

> That isn't so easy because fold is invoked on sizes of types by stor-layout.c 

That's what we should phase out.  The eventual aim should be for (a) 
folding on GIMPLE (gimple-fold etc. - working with SSA not combined trees) 
as an optimization and (b) folding done by front ends only when required 
for language semantics (e.g. constant expressions).  If you want to 
optimize type sizes, look at where they are located in the GIMPLE IR, as a 
property of the IR rather than as a hook checking global state.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [google]: initialize language field for clone function struct

[Eric Botcazou]

> That's what we should phase out.  The eventual aim should be for (a)
> folding on GIMPLE (gimple-fold etc. - working with SSA not combined trees)
> as an optimization and (b) folding done by front ends only when required
> for language semantics (e.g. constant expressions).  If you want to
> optimize type sizes, look at where they are located in the GIMPLE IR, as a
> property of the IR rather than as a hook checking global state.

In Ada we simply don't want to emit raw type sizes in the GIMPLE IR, they are 
just too heavy to be manipulated up to there, even for moderately complex 
code.  That's why we need them to be folded as soon as possible, and even 
factored out of the GENERIC IR into functions when they are self-referential.

-- 
Eric Botcazou

Re: [google]: initialize language field for clone function struct

[Richard Kenner]

> That's what we should phase out.  The eventual aim should be for (a) 
> folding on GIMPLE (gimple-fold etc. - working with SSA not combined trees) 
> as an optimization and (b) folding done by front ends only when required 
> for language semantics (e.g. constant expressions).  

Why?  Isn't it always better to optimize as early as you can when it's
easy?  Why keep unfolded constants around at all?

Re: [google]: initialize language field for clone function struct

[Diego Novillo]

On Wed, May 4, 2011 at 07:44, Richard Kenner <kenner@vlsi1.ultra.nyu.edu> wrote:
>> That's what we should phase out.  The eventual aim should be for (a)
>> folding on GIMPLE (gimple-fold etc. - working with SSA not combined trees)
>> as an optimization and (b) folding done by front ends only when required
>> for language semantics (e.g. constant expressions).
>
> Why?  Isn't it always better to optimize as early as you can when it's
> easy?  Why keep unfolded constants around at all?

There are pros and cons about early optimization, actually.
Generating extremely optimized IL very early can actually tie up
subsequent passes.  For instance, loop unrolling and vectorization.
There are others in the literature.


Diego.

Re: [google]: initialize language field for clone function struct

[Richard Kenner]

> There are pros and cons about early optimization, actually.
> Generating extremely optimized IL very early can actually tie up
> subsequent passes.  For instance, loop unrolling and vectorization.
> There are others in the literature.

Sure, in the sorts of examples you mention where there's a level of
"globality" to it.  But I don't see it in the types of things that
fold does.  How could it ever be better to leave "a - a" in the tree
instead of "0"?

Re: [google]: initialize language field for clone function struct

[Diego Novillo]

On Wed, May 4, 2011 at 08:30, Richard Kenner <kenner@vlsi1.ultra.nyu.edu> wrote:
>> There are pros and cons about early optimization, actually.
>> Generating extremely optimized IL very early can actually tie up
>> subsequent passes.  For instance, loop unrolling and vectorization.
>> There are others in the literature.
>
> Sure, in the sorts of examples you mention where there's a level of
> "globality" to it.  But I don't see it in the types of things that
> fold does.  How could it ever be better to leave "a - a" in the tree
> instead of "0"?

Yes, that's true.


Diego.

Re: [google]: initialize language field for clone function struct

[Michael Matz]

Hi,

On Wed, 4 May 2011, Richard Kenner wrote:

> > There are pros and cons about early optimization, actually.
> > Generating extremely optimized IL very early can actually tie up
> > subsequent passes.  For instance, loop unrolling and vectorization.
> > There are others in the literature.
> 
> Sure, in the sorts of examples you mention where there's a level of
> "globality" to it.  But I don't see it in the types of things that
> fold does.  How could it ever be better to leave "a - a" in the tree
> instead of "0"?

The former is not a constant expression in the C sense, hence folding it 
early disables required diagnostics; that's what Joseph means when he says 
that the frontends should fold only exactly those things that the language 
standard requires, implying not a general catch-all folder.  After the 
frontend is ready with a construct we can do all kinds of foldings as 
early as we like.


Ciao,
Michael.

Re: [google]: initialize language field for clone function struct

[Nathan Froyd]

On Wed, May 04, 2011 at 08:30:40AM -0400, Richard Kenner wrote:
> > There are pros and cons about early optimization, actually.
> > Generating extremely optimized IL very early can actually tie up
> > subsequent passes.  For instance, loop unrolling and vectorization.
> > There are others in the literature.
> 
> Sure, in the sorts of examples you mention where there's a level of
> "globality" to it.  But I don't see it in the types of things that
> fold does.  How could it ever be better to leave "a - a" in the tree
> instead of "0"?

If you ever want to do things beside compile the code--automated
refactoring, renaming, pretty-printing, etc.--fidelity to the source is
preferred.

-Nathan

Re: [google]: initialize language field for clone function struct

[Diego Novillo]

On Wed, May 4, 2011 at 09:19, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On Wed, May 04, 2011 at 08:30:40AM -0400, Richard Kenner wrote:
>> > There are pros and cons about early optimization, actually.
>> > Generating extremely optimized IL very early can actually tie up
>> > subsequent passes.  For instance, loop unrolling and vectorization.
>> > There are others in the literature.
>>
>> Sure, in the sorts of examples you mention where there's a level of
>> "globality" to it.  But I don't see it in the types of things that
>> fold does.  How could it ever be better to leave "a - a" in the tree
>> instead of "0"?
>
> If you ever want to do things beside compile the code--automated
> refactoring, renaming, pretty-printing, etc.--fidelity to the source is
> preferred.

I think we may be talking at different levels.  It's my impression
that Richard K. was referring to local transformations like "a - a" ->
"0" once we are in the middle end.  I agree that doing that
transformation close to the FE is undesirable, but once we are in the
middle end that should be fine.


Diego.

Re: [google]: initialize language field for clone function struct

[Richard Kenner]

> I think we may be talking at different levels.  It's my impression
> that Richard K. was referring to local transformations like "a - a" ->
> "0" once we are in the middle end.  I agree that doing that
> transformation close to the FE is undesirable, but once we are in the
> middle end that should be fine.

That's exactly what I mean.  If the FE cares about no folding taking
place, it shouldn't fold.  But once we're past the point that it's doing
syntax and semantic checking, there's no optimization benefit in deferring
the folding until we get to SSA, for example.

Re: [google]: initialize language field for clone function struct

[Richard Guenther]

On Wed, May 4, 2011 at 12:00 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Umm, I think most of them (if not all) are just bogus.  If a FE doesn't
>> want to fold some stuff when at global scope it should not call fold.
>
> That isn't so easy because fold is invoked on sizes of types by stor-layout.c
> and these sizes can be variable (at least in Ada).  So I think that the calls
> to the hook are still needed.  But:
>  1) The -1 thing should go and the hook return boolean.  The prerequisite is
> to tidy up variable_size,
>  2) I think that the GIMPLE hook can return 0 unconditionally.
>
>> But I'm not sure that is actually what it tries to do ... and the existing
>> checks are far from consistently spread out in fold-const.c ...
>
> It prevents save_expr from being called at global level, since you cannot
> create SAVE_EXPRs outside functions.  Likewise in variable_size.

I see several places in fold-const.c that are not properly guarded then.
But anyway, if it is supposed to protect SAVE_EXPRs then I'd have expected
save_expr to contain that check and possibly return NULL if it can't save.

And I'm not sure you can't do SAVE_EXPRs outside of functions - you
could simply emit global temporaries.

Richard.

> --
> Eric Botcazou
>

Re: [google]: initialize language field for clone function struct

[Michael Matz]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 699 bytes --]

Hi,

On Wed, 4 May 2011, Richard Guenther wrote:

> > It prevents save_expr from being called at global level, since you 
> > cannot create SAVE_EXPRs outside functions.  Likewise in 
> > variable_size.
> 
> I see several places in fold-const.c that are not properly guarded then. 
> But anyway, if it is supposed to protect SAVE_EXPRs then I'd have 
> expected save_expr to contain that check and possibly return NULL if it 
> can't save.
> 
> And I'm not sure you can't do SAVE_EXPRs outside of functions - you 
> could simply emit global temporaries.

It's not the temporary, but the evaluation that poses problems in global 
context.  (you could add a global ctor to it, ugh)


Ciao,
Michael.

Re: [google]: initialize language field for clone function struct

[Richard Guenther]

On Wed, May 4, 2011 at 1:56 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Wed, 4 May 2011, Richard Guenther wrote:
>
>> > It prevents save_expr from being called at global level, since you
>> > cannot create SAVE_EXPRs outside functions.  Likewise in
>> > variable_size.
>>
>> I see several places in fold-const.c that are not properly guarded then.
>> But anyway, if it is supposed to protect SAVE_EXPRs then I'd have
>> expected save_expr to contain that check and possibly return NULL if it
>> can't save.
>>
>> And I'm not sure you can't do SAVE_EXPRs outside of functions - you
>> could simply emit global temporaries.
>
> It's not the temporary, but the evaluation that poses problems in global
> context.  (you could add a global ctor to it, ugh)

Well, a save-expr would only be created if evaluation is necessary anyway,
so I don't see the issue really.

Richard.

Re: [google]: initialize language field for clone function struct

[Eric Botcazou]

> And I'm not sure you can't do SAVE_EXPRs outside of functions - you
> could simply emit global temporaries.

How do you initialize global temporaries with non-trivial initializers?

-- 
Eric Botcazou

Re: [google]: initialize language field for clone function struct

[Richard Guenther]

On Wed, May 4, 2011 at 5:10 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> And I'm not sure you can't do SAVE_EXPRs outside of functions - you
>> could simply emit global temporaries.
>
> How do you initialize global temporaries with non-trivial initializers?

Actually it turns out that it doesn't matter.  If we arrive here with
something that needs a SAVE_EXPR we have to be able to generate code
for it somewhere, where there would be obviously the possibility to
also generate code for a SAVE_EXPR.

No?

Richard.

> --
> Eric Botcazou
>

Re: [google]: initialize language field for clone function struct

[Eric Botcazou]

> Actually it turns out that it doesn't matter.  If we arrive here with
> something that needs a SAVE_EXPR we have to be able to generate code
> for it somewhere, where there would be obviously the possibility to
> also generate code for a SAVE_EXPR.

The transformations done in fold are optimizations that duplicate things, 
hence the need to protect them from multiple evaluations.  If you cannot 
easily do so (e.g. at global level), you just don't do the optimizations.

But, yes, there is something true.  If you have variable sizes at the global 
level, they need to be evaluated once for all (unless self-referential, but 
this is another subject) so you need to do it somewhere.  But you cannot do 
it with SAVE_EXPRs since they would end up being shared across functions.

-- 
Eric Botcazou

Re: [google]: initialize language field for clone function struct

[Richard Guenther]

On Wed, May 4, 2011 at 7:14 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Actually it turns out that it doesn't matter.  If we arrive here with
>> something that needs a SAVE_EXPR we have to be able to generate code
>> for it somewhere, where there would be obviously the possibility to
>> also generate code for a SAVE_EXPR.
>
> The transformations done in fold are optimizations that duplicate things,
> hence the need to protect them from multiple evaluations.  If you cannot
> easily do so (e.g. at global level), you just don't do the optimizations.
>
> But, yes, there is something true.  If you have variable sizes at the global
> level, they need to be evaluated once for all (unless self-referential, but
> this is another subject) so you need to do it somewhere.  But you cannot do
> it with SAVE_EXPRs since they would end up being shared across functions.

Sure, but that's a limitation of out SAVE_EXPR handling (given that it would
be ok to expand the SAVE_EXPR multiple times - once per "instantiation
context").

Richard.

> --
> Eric Botcazou
>

Re: [google]: initialize language field for clone function struct

[Eric Botcazou]

> Sure, but that's a limitation of out SAVE_EXPR handling (given that it
> would be ok to expand the SAVE_EXPR multiple times - once per
> "instantiation context").

You need to expand the initializer exactly once and you need to make sure that 
this occurrence is invoked before all the others at run time.  Not trivial.

-- 
Eric Botcazou

Re: [google]: initialize language field for clone function struct

[Richard Guenther]

On Thu, May 5, 2011 at 11:23 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Sure, but that's a limitation of out SAVE_EXPR handling (given that it
>> would be ok to expand the SAVE_EXPR multiple times - once per
>> "instantiation context").
>
> You need to expand the initializer exactly once and you need to make sure that
> this occurrence is invoked before all the others at run time.  Not trivial.

But where do you expand it without the SAVE_EXPR?  The same
restrictions apply there.  So I suppose you expand it to a function
in which case there is the context where the SAVE_EXPR can be
expanded exactly once.

If you expand it (the "global" expr without SAVE_EXPR) in multiple
function contexts then I see no difference if you have a SAVE_EXPR
and expand that once per such function context.  Yes, this
multiple-times expanding a SAVE_EXPR might not actually work
right now, but it doesn't sound like this would be not fixable
(mark the original SAVE_EXPR as global, during unsharing make
them local and process as usual).

But maybe I'm confused and simply lack an actual testcase that
shows the issue ;)

Richard.

Re: [google]: initialize language field for clone function struct

[Eric Botcazou]

> But where do you expand it without the SAVE_EXPR?  The same
> restrictions apply there.  So I suppose you expand it to a function
> in which case there is the context where the SAVE_EXPR can be
> expanded exactly once.

You don't have SAVE_EXPRs so you're precisely controlling what you're doing.  
Once a SAVE_EXPR is generated, things are pretty much out of control for the 
front-end.

> But maybe I'm confused and simply lack an actual testcase that
> shows the issue ;)

Remove the call to global_bindings_p from variable_size and run gnat.dg.

-- 
Eric Botcazou

Re: [google]: initialize language field for clone function struct

[Richard Guenther]

On Thu, May 5, 2011 at 12:07 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> But where do you expand it without the SAVE_EXPR?  The same
>> restrictions apply there.  So I suppose you expand it to a function
>> in which case there is the context where the SAVE_EXPR can be
>> expanded exactly once.
>
> You don't have SAVE_EXPRs so you're precisely controlling what you're doing.
> Once a SAVE_EXPR is generated, things are pretty much out of control for the
> front-end.
>
>> But maybe I'm confused and simply lack an actual testcase that
>> shows the issue ;)
>
> Remove the call to global_bindings_p from variable_size and run gnat.dg.

Hm, ok.  I was more looking at removing the calls from fold-const.c where
it seems to protect things like folding x <= +Inf to x == x, stuff unlikely
to appear in type sizes.  I can imagine the Ada FE wanting to do
things with type sizes and variable_size's wrapping looks like merely
an optimization to me (for re-using values in nested array types
for example(?)).

Richard.

> --
> Eric Botcazou
>

Re: [google]: initialize language field for clone function struct

[Eric Botcazou]

> Hm, ok.  I was more looking at removing the calls from fold-const.c where
> it seems to protect things like folding x <= +Inf to x == x, stuff unlikely
> to appear in type sizes.  I can imagine the Ada FE wanting to do
> things with type sizes and variable_size's wrapping looks like merely
> an optimization to me (for re-using values in nested array types
> for example(?)).

I indeed can try to remove some/the calls present in fold-const.c as part as 
my upcoming patch to tidy up the hook, but the one present in variable_size 
needs to stay for the time being.

-- 
Eric Botcazou