public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
@ 2008-11-04 21:34 Jakub Jelinek
  2008-11-05 15:52 ` Mark Mitchell
  2008-11-07 22:29 ` Jason Merrill
  0 siblings, 2 replies; 17+ messages in thread
From: Jakub Jelinek @ 2008-11-04 21:34 UTC (permalink / raw)
  To: Jason Merrill, Mark Mitchell; +Cc: gcc-patches

Hi!

As shown on the testcase, sometimes tsubst_decl <case PARM_DECL>:
            if (!DECL_TEMPLATE_PARM_P (r))
              DECL_ARG_TYPE (r) = type_passed_as (type);
us called with still incomplete type, which for (when completed)
TREE_ADDRESSABLE types means DECL_ARG_TYPE is wrong and is passed
that way to the middle-end in CALL_EXPRs current function is making.
DECL_ARG_TYPE will be fixed up later on during
require_complete_types_for_parms when the callee is about to be gimplified,
but that's too late for CALL_CANNOT_INLINE_P/gimple_call_cannot_inline_p.

The following patch fixes this up during genericization of the caller,
but if you have better ideas where to fix this up, I'd like to hear it.
I'm afraid requiring complete type in tsubst_decl for PARM_DECL's type might
be problematic, as that would mean instantiate_template_class in midst
of another instantiate_template_class.

The following patch has been bootstrapped/regtested on x86_64-linux.

2008-11-04  Jakub Jelinek  <jakub@redhat.com>

	PR c++/36631
	* cp-gimplify.c (cp_genericize_r): For FUNCTION_DECLs ensure
	DECL_ARG_TYPE of all PARM_DECLs is correct.

	* g++.dg/template/call5.C: New test.

--- gcc/cp/cp-gimplify.c.jj	2008-10-14 22:11:46.000000000 +0200
+++ gcc/cp/cp-gimplify.c	2008-11-04 17:35:35.000000000 +0100
@@ -790,6 +790,20 @@ cp_genericize_r (tree *stmt_p, int *walk
       default:
 	break;
       }
+
+  /* If PARM_DECLs are tsubsted with incomplete type, DECL_ARG_TYPE might
+     be wrong for functions this function calls or otherwise references.
+     Update it here.  */
+  else if (TREE_CODE (stmt) == FUNCTION_DECL
+	   && DECL_TEMPLATE_INFO (stmt))
+    {
+      tree t;
+      for (t = DECL_ARGUMENTS (stmt); t; t = TREE_CHAIN (t))
+	if (TREE_ADDRESSABLE (TREE_TYPE (t)) && !DECL_BY_REFERENCE (t))
+	  DECL_ARG_TYPE (t) = type_passed_as (TREE_TYPE (t));
+      *walk_subtrees = 0;
+    }
+
   else if (IS_TYPE_OR_DECL_P (stmt))
     *walk_subtrees = 0;
 
--- gcc/testsuite/g++.dg/template/call5.C.jj	2008-11-04 17:50:34.000000000 +0100
+++ gcc/testsuite/g++.dg/template/call5.C	2008-11-04 17:49:46.000000000 +0100
@@ -0,0 +1,17 @@
+// PR c++/36631
+// { dg-options "-O0" }
+
+template <typename T> struct B
+{
+  struct C
+  {
+    __attribute__ ((always_inline)) C (C const &c) {}
+  };
+  void __attribute__ ((always_inline)) g (C c) {}
+};
+
+void
+trigger (B <int> b, B <int>::C c)
+{
+  b.g (c);
+}

	Jakub

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-04 21:34 [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631) Jakub Jelinek
@ 2008-11-05 15:52 ` Mark Mitchell
  2008-11-06  9:17   ` Jakub Jelinek
  2008-11-07 22:29 ` Jason Merrill
  1 sibling, 1 reply; 17+ messages in thread
From: Mark Mitchell @ 2008-11-05 15:52 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Jason Merrill, gcc-patches

Jakub Jelinek wrote:

> The following patch has been bootstrapped/regtested on x86_64-linux.
> 
> 2008-11-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/36631
> 	* cp-gimplify.c (cp_genericize_r): For FUNCTION_DECLs ensure
> 	DECL_ARG_TYPE of all PARM_DECLs is correct.
> 
> 	* g++.dg/template/call5.C: New test.

Why not just defer *all* computation of DECL_ARG_TYPE until converting
to generic?  I don't think the C++ front-end actually uses it, and I
don't think it should.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-05 15:52 ` Mark Mitchell
@ 2008-11-06  9:17   ` Jakub Jelinek
  2008-11-06 10:05     ` Richard Guenther
  2008-11-09 18:03     ` Mark Mitchell
  0 siblings, 2 replies; 17+ messages in thread
From: Jakub Jelinek @ 2008-11-06  9:17 UTC (permalink / raw)
  To: Mark Mitchell; +Cc: Jason Merrill, gcc-patches

On Wed, Nov 05, 2008 at 07:51:23AM -0800, Mark Mitchell wrote:
> Jakub Jelinek wrote:
> 
> > The following patch has been bootstrapped/regtested on x86_64-linux.
> > 
> > 2008-11-04  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR c++/36631
> > 	* cp-gimplify.c (cp_genericize_r): For FUNCTION_DECLs ensure
> > 	DECL_ARG_TYPE of all PARM_DECLs is correct.
> > 
> > 	* g++.dg/template/call5.C: New test.
> 
> Why not just defer *all* computation of DECL_ARG_TYPE until converting
> to generic?  I don't think the C++ front-end actually uses it, and I
> don't think it should.

Not all functions go through conversion to generic, nor are referenced by
some function during genericization, say in
struct A { A (); ~A (); };
void foo (A, A, A);
static void (*fnptr) (A, A, A) = foo;
int main ()
{
  fnptr (A (), A(), A());
}
where would DECL_ARG_TYPE be set for foo?  Only during optimizations
GCC figures out that fnptr is actually const (being static and never
modified in the CU) and constant propagates it.

	Jakub

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-06  9:17   ` Jakub Jelinek
@ 2008-11-06 10:05     ` Richard Guenther
  2008-11-06 10:28       ` Jakub Jelinek
  2008-11-09 18:03     ` Mark Mitchell
  1 sibling, 1 reply; 17+ messages in thread
From: Richard Guenther @ 2008-11-06 10:05 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Mark Mitchell, Jason Merrill, gcc-patches

On Thu, Nov 6, 2008 at 10:14 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Nov 05, 2008 at 07:51:23AM -0800, Mark Mitchell wrote:
>> Jakub Jelinek wrote:
>>
>> > The following patch has been bootstrapped/regtested on x86_64-linux.
>> >
>> > 2008-11-04  Jakub Jelinek  <jakub@redhat.com>
>> >
>> >     PR c++/36631
>> >     * cp-gimplify.c (cp_genericize_r): For FUNCTION_DECLs ensure
>> >     DECL_ARG_TYPE of all PARM_DECLs is correct.
>> >
>> >     * g++.dg/template/call5.C: New test.
>>
>> Why not just defer *all* computation of DECL_ARG_TYPE until converting
>> to generic?  I don't think the C++ front-end actually uses it, and I
>> don't think it should.
>
> Not all functions go through conversion to generic, nor are referenced by
> some function during genericization, say in
> struct A { A (); ~A (); };
> void foo (A, A, A);
> static void (*fnptr) (A, A, A) = foo;
> int main ()
> {
>  fnptr (A (), A(), A());
> }
> where would DECL_ARG_TYPE be set for foo?  Only during optimizations
> GCC figures out that fnptr is actually const (being static and never
> modified in the CU) and constant propagates it.

Can we do it from the gimplification langhook?

Richard.

>        Jakub
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-06 10:05     ` Richard Guenther
@ 2008-11-06 10:28       ` Jakub Jelinek
  0 siblings, 0 replies; 17+ messages in thread
From: Jakub Jelinek @ 2008-11-06 10:28 UTC (permalink / raw)
  To: Richard Guenther; +Cc: Mark Mitchell, Jason Merrill, gcc-patches

On Thu, Nov 06, 2008 at 11:04:40AM +0100, Richard Guenther wrote:
> >> Why not just defer *all* computation of DECL_ARG_TYPE until converting
> >> to generic?  I don't think the C++ front-end actually uses it, and I
> >> don't think it should.
> >
> > Not all functions go through conversion to generic, nor are referenced by
> > some function during genericization, say in
> > struct A { A (); ~A (); };
> > void foo (A, A, A);
> > static void (*fnptr) (A, A, A) = foo;
> > int main ()
> > {
> >  fnptr (A (), A(), A());
> > }
> > where would DECL_ARG_TYPE be set for foo?  Only during optimizations
> > GCC figures out that fnptr is actually const (being static and never
> > modified in the CU) and constant propagates it.
> 
> Can we do it from the gimplification langhook?

That doesn't help at all, nothing is gimplified that isn't genericized
first.  But DECL_INITIAL of static vars isn't genericized nor gimplified.

	Jakub

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-04 21:34 [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631) Jakub Jelinek
  2008-11-05 15:52 ` Mark Mitchell
@ 2008-11-07 22:29 ` Jason Merrill
  2008-11-08  3:01   ` Jakub Jelinek
  1 sibling, 1 reply; 17+ messages in thread
From: Jason Merrill @ 2008-11-07 22:29 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Mark Mitchell, gcc-patches

Jakub Jelinek wrote:
> I'm afraid requiring complete type in tsubst_decl for PARM_DECL's type might
> be problematic, as that would mean instantiate_template_class in midst
> of another instantiate_template_class.

I'm pretty sure that already happens all the time.

Jason

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-07 22:29 ` Jason Merrill
@ 2008-11-08  3:01   ` Jakub Jelinek
  2008-11-08  9:05     ` Jason Merrill
  0 siblings, 1 reply; 17+ messages in thread
From: Jakub Jelinek @ 2008-11-08  3:01 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Mark Mitchell, gcc-patches

On Fri, Nov 07, 2008 at 04:13:53PM -0500, Jason Merrill wrote:
> Jakub Jelinek wrote:
>> I'm afraid requiring complete type in tsubst_decl for PARM_DECL's type might
>> be problematic, as that would mean instantiate_template_class in midst
>> of another instantiate_template_class.
>
> I'm pretty sure that already happens all the time.

Adding:
--- gcc/cp/pt.c	2008-10-14 22:09:53.000000000 +0200
+++ gcc/cp/pt.c	2008-11-07 23:18:35.000000000 +0100
@@ -8355,7 +8355,10 @@ tsubst_decl (tree t, tree args, tsubst_f
             DECL_CONTEXT (r) = NULL_TREE;
 
             if (!DECL_TEMPLATE_PARM_P (r))
-              DECL_ARG_TYPE (r) = type_passed_as (type);
+	      {
+		complete_type (type);
+		DECL_ARG_TYPE (r) = type_passed_as (type);
+	      }
 
 	    apply_late_template_attributes (&r, DECL_ATTRIBUTES (r), 0,
 					    args, complain, in_decl);
might help in a few cases (e.g. on the testcase from the PR), but certainly
not all.  Say:

struct C
{
  C ();
  void foo (C, C);
};

void bar (void)
{
  C c;
  c.foo (c, c);
}

template <typename T> struct D
{
  D ();
  void foo (D, D);
};

void baz (void)
{
  D<int> d;
  d.foo (d, d);
}

When type_passed_as is called for C::foo, C is still not complete and
complete_type doesn't do anything on it, when the above complete_type added
to tsubst_decl is called on D, D is TYPE_BEING_DEFINED and thus
complete_type doesn't do anything either.  So on this testcase,
with or without the patch, the middle end gets incorrect DECL_ARG_TYPE
on the last 2 args of all foo methods.

	Jakub

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-08  3:01   ` Jakub Jelinek
@ 2008-11-08  9:05     ` Jason Merrill
  2008-11-08  9:38       ` Jakub Jelinek
  0 siblings, 1 reply; 17+ messages in thread
From: Jason Merrill @ 2008-11-08  9:05 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Mark Mitchell, gcc-patches

I don't understand why the caller cares about DECL_ARG_TYPE.  I'll dig 
into this some more.

Jason

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-08  9:05     ` Jason Merrill
@ 2008-11-08  9:38       ` Jakub Jelinek
  0 siblings, 0 replies; 17+ messages in thread
From: Jakub Jelinek @ 2008-11-08  9:38 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Mark Mitchell, gcc-patches

On Sat, Nov 08, 2008 at 01:51:06AM -0500, Jason Merrill wrote:
> I don't understand why the caller cares about DECL_ARG_TYPE.  I'll dig  
> into this some more.

Better testcase is maybe:

// { dg-options "-O2 -Winline" }
template <typename T>
struct C
{
  C ();
  C (const C &);
  ~C ();
  inline __attribute__((always_inline)) void foo (C, C);
};

void bar (C<int> c)
{
  c.foo (c, c);
}

template <typename T>
inline __attribute__((always_inline)) void C<T>::foo (C, C)
{
}

Again, when foo PARM_DECLs are tsubsted using tsubst_decl, C is
incomplete (and is TYPE_BEING_DEFINED, so complete_type doesn't help)
and nothing fixes up DECL_ARG_TYPE until C<int>::foo is genericized.
Before that bar is genericized and gimplified, and triggers:
  /* Verify if the type of the argument matches that of the function
     declaration.  If we cannot verify this or there is a mismatch,
     mark the call expression so it doesn't get inlined later.  */
  if (fndecl && DECL_ARGUMENTS (fndecl))
    {
      for (i = 0, p = DECL_ARGUMENTS (fndecl);
           i < nargs;
           i++, p = TREE_CHAIN (p))
        {
          /* We cannot distinguish a varargs function from the case
             of excess parameters, still deferring the inlining decision
             to the callee is possible.  */
          if (!p)
            break;
          if (p == error_mark_node
              || CALL_EXPR_ARG (*expr_p, i) == error_mark_node
              || !fold_convertible_p (DECL_ARG_TYPE (p),
                                      CALL_EXPR_ARG (*expr_p, i)))
            {
              CALL_CANNOT_INLINE_P (*expr_p) = 1;
              break;
            }
        }
    }
in gimplify_call_expr (the call argument is REFERENCE_TYPE, but
DECL_ARG_TYPE incorrectly RECORD_TYPE (with TREE_ADDRESSABLE set).
The reason why we don't want to inline calls with mismatching
types is that it horribly confuses the inliner, we often ICE on it. 
Consider say (in C) nonportable code like:
inline int foo ();

int bar (void)
{
  return foo (64LL);
}

inline int foo (int i, int j)
{
  return i + j;
}
on i?86-*-*.  We really don't want to inline in this case.
Afterwards C<int>::foo is genericized and DECL_ARG_TYPE is fixed up,
so when we actually throw all the functions into cgraph's hands, it
actually would be able to inline it, but gimple_call_cannot_inline_p
is already set at this point.

	Jakub

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-06  9:17   ` Jakub Jelinek
  2008-11-06 10:05     ` Richard Guenther
@ 2008-11-09 18:03     ` Mark Mitchell
  2008-11-10 22:55       ` Jason Merrill
  1 sibling, 1 reply; 17+ messages in thread
From: Mark Mitchell @ 2008-11-09 18:03 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Jason Merrill, gcc-patches

Jakub Jelinek wrote:
> On Wed, Nov 05, 2008 at 07:51:23AM -0800, Mark Mitchell wrote:
>> Jakub Jelinek wrote:
>>
>>> The following patch has been bootstrapped/regtested on x86_64-linux.
>>>
>>> 2008-11-04  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	PR c++/36631
>>> 	* cp-gimplify.c (cp_genericize_r): For FUNCTION_DECLs ensure
>>> 	DECL_ARG_TYPE of all PARM_DECLs is correct.
>>>
>>> 	* g++.dg/template/call5.C: New test.
>> Why not just defer *all* computation of DECL_ARG_TYPE until converting
>> to generic?  I don't think the C++ front-end actually uses it, and I
>> don't think it should.
> 
> Not all functions go through conversion to generic, nor are referenced by
> some function during genericization

That may be -- but I think the basic point remains true.  DECL_ARG_TYPE
is not a front-end concept.  It's an implementation detail, describing
how exactly the argument is passed.  As such, it really should be left
to the middle-end to compute as needed.

The implementation of type_passed_as in the C++ front-end doesn't appear
to use any functionality specific to the front-end.  How about just
computing DECL_ARG_TYPE lazily when something needs it?

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-09 18:03     ` Mark Mitchell
@ 2008-11-10 22:55       ` Jason Merrill
  2008-11-10 22:57         ` Mark Mitchell
  0 siblings, 1 reply; 17+ messages in thread
From: Jason Merrill @ 2008-11-10 22:55 UTC (permalink / raw)
  To: Mark Mitchell; +Cc: Jakub Jelinek, gcc-patches

Mark Mitchell wrote:
> That may be -- but I think the basic point remains true.  DECL_ARG_TYPE
> is not a front-end concept.  It's an implementation detail, describing
> how exactly the argument is passed.  As such, it really should be left
> to the middle-end to compute as needed.

Well, it depends on the ABI, which can be language-specific.

The issue is much like the one that motivated complete_vars: we can 
treat a class as complete while we're in the middle of the definition, 
but when we're done defining the class we need to go back and fix up the 
places which really need bits that we don't know until after 
finish_struct.  That can be done either toward the end of finish_struct, 
as with complete_vars, or as needed later on.  For functions being 
defined, we update DECL_ARG_TYPE as part of start_function 
(require_complete_type_for_parms).  Gimplification seems to be a fine 
time to do this for called functions.

I don't think this issue is specific to templates, however; the example 
in one of Jakub's mails

struct C
{
   C ();
   void foo (C, C);
};

void bar (void)
{
   C c;
   c.foo (c, c);
}

I would expect to have the same problem.

Jason

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-10 22:55       ` Jason Merrill
@ 2008-11-10 22:57         ` Mark Mitchell
  2008-11-10 23:28           ` Jakub Jelinek
  2008-11-19  3:46           ` Jason Merrill
  0 siblings, 2 replies; 17+ messages in thread
From: Mark Mitchell @ 2008-11-10 22:57 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Jakub Jelinek, gcc-patches

Jason Merrill wrote:
> Mark Mitchell wrote:
>> That may be -- but I think the basic point remains true.  DECL_ARG_TYPE
>> is not a front-end concept.  It's an implementation detail, describing
>> how exactly the argument is passed.  As such, it really should be left
>> to the middle-end to compute as needed.
> 
> Well, it depends on the ABI, which can be language-specific.

Indeed, but this seems to me like an appropriate place for a langhook,
if one is required.  It's language-specific information that's required
as part of the lowering process from language-specific trees to GENERIC
and/or GIMPLE.

I guess my basic point is that by deferring this until that point, we're
guaranteed to avoid problems with incomplete types.  If we try to do it
before the translation unit is completely processed, then we're at risk
of getting it wrong.  So, a lazy approach (whether by making
DECL_ARG_TYPE itself actually invoke a hook if the value is NULL, or by
doing it at the point of gimplification, or some other strategy) seems
safest.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-10 22:57         ` Mark Mitchell
@ 2008-11-10 23:28           ` Jakub Jelinek
  2008-11-19  3:46           ` Jason Merrill
  1 sibling, 0 replies; 17+ messages in thread
From: Jakub Jelinek @ 2008-11-10 23:28 UTC (permalink / raw)
  To: Mark Mitchell; +Cc: Jason Merrill, gcc-patches

On Mon, Nov 10, 2008 at 02:44:53PM -0800, Mark Mitchell wrote:
> Jason Merrill wrote:
> > Mark Mitchell wrote:
> >> That may be -- but I think the basic point remains true.  DECL_ARG_TYPE
> >> is not a front-end concept.  It's an implementation detail, describing
> >> how exactly the argument is passed.  As such, it really should be left
> >> to the middle-end to compute as needed.
> > 
> > Well, it depends on the ABI, which can be language-specific.
> 
> Indeed, but this seems to me like an appropriate place for a langhook,
> if one is required.  It's language-specific information that's required
> as part of the lowering process from language-specific trees to GENERIC
> and/or GIMPLE.
> 
> I guess my basic point is that by deferring this until that point, we're
> guaranteed to avoid problems with incomplete types.  If we try to do it
> before the translation unit is completely processed, then we're at risk
> of getting it wrong.  So, a lazy approach (whether by making
> DECL_ARG_TYPE itself actually invoke a hook if the value is NULL, or by
> doing it at the point of gimplification, or some other strategy) seems
> safest.

We also have an option to not check this at gimplification time, but later
on (e.g. during GIMPLE lowering).  It just has to be done before early
inlining.  GIMPLE lowering is called from within cgraph_optimize, so
we might just go through all the FUNCTION_DECLs we've created at
cp_write_global_declarations time and compute DECL_ARG_TYPE for all their
arguments at that point.

	Jakub

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-10 22:57         ` Mark Mitchell
  2008-11-10 23:28           ` Jakub Jelinek
@ 2008-11-19  3:46           ` Jason Merrill
  2008-11-19  8:38             ` Jakub Jelinek
  1 sibling, 1 reply; 17+ messages in thread
From: Jason Merrill @ 2008-11-19  3:46 UTC (permalink / raw)
  To: Mark Mitchell; +Cc: Jakub Jelinek, gcc-patches

I note that the code in gimplify_call_expr is the only place in the 
compiler that checks the DECL_ARG_TYPE of a called function; everywhere 
else only uses it for the parameters of the function currently being 
compiled.  This is why this mismatch has not been a problem in the past. 
  Perhaps gimplify_call_expr shouldn't be relying on it?

Jason

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-19  3:46           ` Jason Merrill
@ 2008-11-19  8:38             ` Jakub Jelinek
  2008-11-19 14:34               ` Jakub Jelinek
  0 siblings, 1 reply; 17+ messages in thread
From: Jakub Jelinek @ 2008-11-19  8:38 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Mark Mitchell, gcc-patches

On Tue, Nov 18, 2008 at 07:00:29PM -0500, Jason Merrill wrote:
> I note that the code in gimplify_call_expr is the only place in the  
> compiler that checks the DECL_ARG_TYPE of a called function; everywhere  
> else only uses it for the parameters of the function currently being  
> compiled.  This is why this mismatch has not been a problem in the past.  
>  Perhaps gimplify_call_expr shouldn't be relying on it?

Using just TREE_TYPE of the PARM_DECL isn't any better with the C++ FE,
that's also incorrect until the function has been through
cp_genericize.  And it would be wrong for types smaller than int
which are promoted to int.

As I said earlier, I can easily move this checking from gimplify_call_expr
until gimple lowering, at which point the C++ FE should be through
cp_genericize with most of the functions (exceptions are late generated
functions, e.g. during OpenMP lowering, but those can handle DECL_ARG_TYPE
explicitly in the langhooks).  This would make DECL_ARG_TYPE correct
for all functions that can be inlined (where bodies exist), but still
DECL_ARG_TYPE (and TREE_TYPE) of PARM_DECLs would be wrong for DECL_EXTERNAL
functions.

	Jakub

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-19  8:38             ` Jakub Jelinek
@ 2008-11-19 14:34               ` Jakub Jelinek
  2008-11-22  2:40                 ` Ian Lance Taylor
  0 siblings, 1 reply; 17+ messages in thread
From: Jakub Jelinek @ 2008-11-19 14:34 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Mark Mitchell, gcc-patches

On Wed, Nov 19, 2008 at 09:06:37AM +0100, Jakub Jelinek wrote:
> On Tue, Nov 18, 2008 at 07:00:29PM -0500, Jason Merrill wrote:
> > I note that the code in gimplify_call_expr is the only place in the  
> > compiler that checks the DECL_ARG_TYPE of a called function; everywhere  
> > else only uses it for the parameters of the function currently being  
> > compiled.  This is why this mismatch has not been a problem in the past.  
> >  Perhaps gimplify_call_expr shouldn't be relying on it?
> 
> Using just TREE_TYPE of the PARM_DECL isn't any better with the C++ FE,
> that's also incorrect until the function has been through
> cp_genericize.  And it would be wrong for types smaller than int
> which are promoted to int.
> 
> As I said earlier, I can easily move this checking from gimplify_call_expr
> until gimple lowering, at which point the C++ FE should be through
> cp_genericize with most of the functions (exceptions are late generated
> functions, e.g. during OpenMP lowering, but those can handle DECL_ARG_TYPE
> explicitly in the langhooks).  This would make DECL_ARG_TYPE correct
> for all functions that can be inlined (where bodies exist), but still
> DECL_ARG_TYPE (and TREE_TYPE) of PARM_DECLs would be wrong for DECL_EXTERNAL
> functions.

Here is a patch which does that.  Bootstrapped/regtested on x86_64-linux,
fixes the testcase.  Still for DECL_EXTERNAL routines DECL_ARG_TYPE and
TREE_TYPE of PARM_DECLs will be incorrect, which doesn't affect inlining (as
we don't have a body), but is still wrong.

2008-11-19  Jakub Jelinek  <jakub@redhat.com>

	PR c++/36631
	* gimplify.c (gimplify_call_expr): Defer most of the cannot inline
	checking until GIMPLE lowering.
	* gimple-low.c (check_call_args): New function.
	(lower_stmt) <case GIMPLE_CALL>: Call it.

	* g++.dg/template/call5.C: New test.
	
--- gcc/gimplify.c.jj	2008-11-18 19:24:09.000000000 +0100
+++ gcc/gimplify.c	2008-11-19 11:23:06.000000000 +0100
@@ -2352,56 +2352,18 @@ gimplify_call_expr (tree *expr_p, gimple
   else if (POINTER_TYPE_P (TREE_TYPE (CALL_EXPR_FN (*expr_p))))
     parms = TYPE_ARG_TYPES (TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (*expr_p))));
 
-  /* Verify if the type of the argument matches that of the function
-     declaration.  If we cannot verify this or there is a mismatch,
-     mark the call expression so it doesn't get inlined later.  */
   if (fndecl && DECL_ARGUMENTS (fndecl))
-    {
-      for (i = 0, p = DECL_ARGUMENTS (fndecl);
-	   i < nargs;
-	   i++, p = TREE_CHAIN (p))
-	{
-	  /* We cannot distinguish a varargs function from the case
-	     of excess parameters, still deferring the inlining decision
-	     to the callee is possible.  */
-	  if (!p)
-	    break;
-	  if (p == error_mark_node
-	      || CALL_EXPR_ARG (*expr_p, i) == error_mark_node
-	      || !fold_convertible_p (DECL_ARG_TYPE (p),
-				      CALL_EXPR_ARG (*expr_p, i)))
-	    {
-	      CALL_CANNOT_INLINE_P (*expr_p) = 1;
-	      break;
-	    }
-	}
-    }
+    p = DECL_ARGUMENTS (fndecl);
   else if (parms)
-    {
-      for (i = 0, p = parms; i < nargs; i++, p = TREE_CHAIN (p))
-	{
-	  /* If this is a varargs function defer inlining decision
-	     to callee.  */
-	  if (!p)
-	    break;
-	  if (TREE_VALUE (p) == error_mark_node
-	      || CALL_EXPR_ARG (*expr_p, i) == error_mark_node
-	      || TREE_CODE (TREE_VALUE (p)) == VOID_TYPE
-	      || !fold_convertible_p (TREE_VALUE (p),
-				      CALL_EXPR_ARG (*expr_p, i)))
-	    {
-	      CALL_CANNOT_INLINE_P (*expr_p) = 1;
-	      break;
-	    }
-	}
-    }
+    p = parms;
   else
     {
       if (nargs != 0)
 	CALL_CANNOT_INLINE_P (*expr_p) = 1;
-      i = 0;
       p = NULL_TREE;
     }
+  for (i = 0; i < nargs && p; i++, p = TREE_CHAIN (p))
+    ;
 
   /* If the last argument is __builtin_va_arg_pack () and it is not
      passed as a named argument, decrease the number of CALL_EXPR
--- gcc/gimple-low.c.jj	2008-10-23 13:21:39.000000000 +0200
+++ gcc/gimple-low.c	2008-11-19 10:19:28.000000000 +0100
@@ -1,6 +1,7 @@
 /* GIMPLE lowering pass.  Converts High GIMPLE into Low GIMPLE.
 
-   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008 Free Software Foundation, Inc.
+   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008
+   Free Software Foundation, Inc.
 
 This file is part of GCC.
 
@@ -218,6 +219,80 @@ struct gimple_opt_pass pass_lower_cf = 
 };
 
 
+/* Verify if the type of the argument matches that of the function
+   declaration.  If we cannot verify this or there is a mismatch,
+   mark the call expression so it doesn't get inlined later.  */
+
+static void
+check_call_args (gimple stmt)
+{
+  tree fndecl, parms, p;
+  unsigned int i, nargs;
+
+  if (gimple_call_cannot_inline_p (stmt))
+    return;
+
+  nargs = gimple_call_num_args (stmt);
+
+  /* Get argument types for verification.  */
+  fndecl = gimple_call_fndecl (stmt);
+  parms = NULL_TREE;
+  if (fndecl)
+    parms = TYPE_ARG_TYPES (TREE_TYPE (fndecl));
+  else if (POINTER_TYPE_P (TREE_TYPE (gimple_call_fn (stmt))))
+    parms = TYPE_ARG_TYPES (TREE_TYPE (TREE_TYPE (gimple_call_fn (stmt))));
+
+  /* Verify if the type of the argument matches that of the function
+     declaration.  If we cannot verify this or there is a mismatch,
+     mark the call expression so it doesn't get inlined later.  */
+  if (fndecl && DECL_ARGUMENTS (fndecl))
+    {
+      for (i = 0, p = DECL_ARGUMENTS (fndecl);
+	   i < nargs;
+	   i++, p = TREE_CHAIN (p))
+	{
+	  /* We cannot distinguish a varargs function from the case
+	     of excess parameters, still deferring the inlining decision
+	     to the callee is possible.  */
+	  if (!p)
+	    break;
+	  if (p == error_mark_node
+	      || gimple_call_arg (stmt, i) == error_mark_node
+	      || !fold_convertible_p (DECL_ARG_TYPE (p),
+				      gimple_call_arg (stmt, i)))
+	    {
+	      gimple_call_set_cannot_inline (stmt, true);
+	      break;
+	    }
+	}
+    }
+  else if (parms)
+    {
+      for (i = 0, p = parms; i < nargs; i++, p = TREE_CHAIN (p))
+	{
+	  /* If this is a varargs function defer inlining decision
+	     to callee.  */
+	  if (!p)
+	    break;
+	  if (TREE_VALUE (p) == error_mark_node
+	      || gimple_call_arg (stmt, i) == error_mark_node
+	      || TREE_CODE (TREE_VALUE (p)) == VOID_TYPE
+	      || !fold_convertible_p (TREE_VALUE (p),
+				      gimple_call_arg (stmt, i)))
+	    {
+	      gimple_call_set_cannot_inline (stmt, true);
+	      break;
+	    }
+	}
+    }
+  else
+    {
+      if (nargs != 0)
+	gimple_call_set_cannot_inline (stmt, true);
+    }
+}
+
+
 /* Lower sequence SEQ.  Unlike gimplification the statements are not relowered
    when they are changed -- if this has to be done, the lowering routine must
    do it explicitly.  DATA is passed through the recursion.  */
@@ -320,6 +395,7 @@ lower_stmt (gimple_stmt_iterator *gsi, s
 	    lower_builtin_setjmp (gsi);
 	    return;
 	  }
+	check_call_args (stmt);
       }
       break;
 
--- gcc/testsuite/g++.dg/template/call5.C.jj	2008-11-19 11:44:13.000000000 +0100
+++ gcc/testsuite/g++.dg/template/call5.C	2008-11-19 11:38:27.000000000 +0100
@@ -0,0 +1,17 @@
+// PR c++/36631
+// { dg-options "-O0" }
+
+template <typename T> struct B
+{
+  struct C
+  {
+    __attribute__ ((always_inline)) C (C const &c) {}
+  };
+  void __attribute__ ((always_inline)) g (C c) {}
+};
+
+void
+trigger (B <int> b, B <int>::C c)
+{
+  b.g (c);
+}


	Jakub

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631)
  2008-11-19 14:34               ` Jakub Jelinek
@ 2008-11-22  2:40                 ` Ian Lance Taylor
  0 siblings, 0 replies; 17+ messages in thread
From: Ian Lance Taylor @ 2008-11-22  2:40 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Jason Merrill, Mark Mitchell, gcc-patches

Jakub Jelinek <jakub@redhat.com> writes:

> 2008-11-19  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR c++/36631
> 	* gimplify.c (gimplify_call_expr): Defer most of the cannot inline
> 	checking until GIMPLE lowering.
> 	* gimple-low.c (check_call_args): New function.
> 	(lower_stmt) <case GIMPLE_CALL>: Call it.
>
> 	* g++.dg/template/call5.C: New test.

This is OK.

Thanks.

Ian

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2008-11-22  2:33 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-11-04 21:34 [C++ PATCH] Fix up DECL_ARG_TYPE (PR c++/36631) Jakub Jelinek
2008-11-05 15:52 ` Mark Mitchell
2008-11-06  9:17   ` Jakub Jelinek
2008-11-06 10:05     ` Richard Guenther
2008-11-06 10:28       ` Jakub Jelinek
2008-11-09 18:03     ` Mark Mitchell
2008-11-10 22:55       ` Jason Merrill
2008-11-10 22:57         ` Mark Mitchell
2008-11-10 23:28           ` Jakub Jelinek
2008-11-19  3:46           ` Jason Merrill
2008-11-19  8:38             ` Jakub Jelinek
2008-11-19 14:34               ` Jakub Jelinek
2008-11-22  2:40                 ` Ian Lance Taylor
2008-11-07 22:29 ` Jason Merrill
2008-11-08  3:01   ` Jakub Jelinek
2008-11-08  9:05     ` Jason Merrill
2008-11-08  9:38       ` Jakub Jelinek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).