[Patch, middle-end] Fix PR 44878, IA64 build failure, partial inlining

[Patch, middle-end] Fix PR 44878, IA64 build failure, partial inlining

[Steve Ellcey]

I have not been able to bootstrap GCC on ia64-hp-hpux11.23 since the
partial inlining change went in.  Even if I set flag_partial_inlining
to zero I cannot build.  This patch fixes the build enough so that I
can bootstrap with flag_partial_inlining set to zero but it still does
not allow me to build with partial inlining turned on (PR 44716).

The problem I am running into when flag_partial_inlining is zero  is the
'!DECL_BY_REFERENCE (t)' check that was added to
"needs_to_live_in_memory()" during the partial inlining checking
(r161898).  With this check in place I  get an ICE and without it things
work fine so I would like to remove it.  It doesn't seem to affect any
of my other builds (including x86) but it does break the IA64 build.

Looking at the code it seems odd in that we now have (in
needs_to_live_in_memory):

   return (TREE_ADDRESSABLE (t)
          || is_global_var (t)
          || (TREE_CODE (t) == RESULT_DECL
              && !DECL_BY_REFERENCE (t)
              && aggregate_value_p (t, current_function_decl)));

And inside 'aggregate_value_p' we have:

	  /* If the front end has decided that this needs to be passed by
             reference, do so.  */
          if ((TREE_CODE (exp) == PARM_DECL || TREE_CODE (exp) == RESULT_DECL)
              && DECL_BY_REFERENCE (exp))
            return 1;

So if aggregate_value_p is going out of its way to return TRUE for 
DECL_BY_REFERENCE why is needs_to_live_in_memory going out of its way to
return FALSE?  I think if something is being returned by reference it
does need to live in memory.  The reference itself does not need to live
in memory but the thing it is referencing does and I think we are talking
here about the thing being referenced, not the reference itself.

Tested on IA64 HP-UX and Linux and on x86 Linux.  OK for checkin?

Steve Ellcey
sje@cup.hp.com



2010-07-16  Steve Ellcey  <sje@cup.hp.com>

	PR middle-end/44878
	* tree.c (needs_to_live_in_memory):  Remove DECL_BY_REFERENCE check.


Index: tree.c
===================================================================
--- tree.c	(revision 162239)
+++ tree.c	(working copy)
@@ -9741,7 +9741,6 @@ needs_to_live_in_memory (const_tree t)
   return (TREE_ADDRESSABLE (t)
 	  || is_global_var (t)
 	  || (TREE_CODE (t) == RESULT_DECL
-	      && !DECL_BY_REFERENCE (t)
 	      && aggregate_value_p (t, current_function_decl)));
 }
 

Re: [Patch, middle-end] Fix PR 44878, IA64 build failure, partial inlining

[Richard Guenther]

On Fri, Jul 16, 2010 at 10:23 PM, Steve Ellcey <sje@cup.hp.com> wrote:
> I have not been able to bootstrap GCC on ia64-hp-hpux11.23 since the
> partial inlining change went in.  Even if I set flag_partial_inlining
> to zero I cannot build.  This patch fixes the build enough so that I
> can bootstrap with flag_partial_inlining set to zero but it still does
> not allow me to build with partial inlining turned on (PR 44716).
>
> The problem I am running into when flag_partial_inlining is zero  is the
> '!DECL_BY_REFERENCE (t)' check that was added to
> "needs_to_live_in_memory()" during the partial inlining checking
> (r161898).  With this check in place I  get an ICE and without it things
> work fine so I would like to remove it.  It doesn't seem to affect any
> of my other builds (including x86) but it does break the IA64 build.
>
> Looking at the code it seems odd in that we now have (in
> needs_to_live_in_memory):
>
>   return (TREE_ADDRESSABLE (t)
>          || is_global_var (t)
>          || (TREE_CODE (t) == RESULT_DECL
>              && !DECL_BY_REFERENCE (t)
>              && aggregate_value_p (t, current_function_decl)));
>
> And inside 'aggregate_value_p' we have:
>
>          /* If the front end has decided that this needs to be passed by
>             reference, do so.  */
>          if ((TREE_CODE (exp) == PARM_DECL || TREE_CODE (exp) == RESULT_DECL)
>              && DECL_BY_REFERENCE (exp))
>            return 1;
>
> So if aggregate_value_p is going out of its way to return TRUE for
> DECL_BY_REFERENCE why is needs_to_live_in_memory going out of its way to
> return FALSE?  I think if something is being returned by reference it
> does need to live in memory.  The reference itself does not need to live
> in memory but the thing it is referencing does and I think we are talking
> here about the thing being referenced, not the reference itself.

No, we are talking about the reference here.  See

/* In a RESULT_DECL, PARM_DECL and VAR_DECL, means that it is
   passed by invisible reference (and the TREE_TYPE is a pointer to the true
   type).  */
#define DECL_BY_REFERENCE(NODE) \

"TREE_TYPE is a pointer to the true type"

The problem might be that the needs_to_live_in_memory predicate ends up
being used in incompatible contexts.  Which of the callers ends up
making the difference to you?

Richard.

> Tested on IA64 HP-UX and Linux and on x86 Linux.  OK for checkin?
>
> Steve Ellcey
> sje@cup.hp.com
>
>
>
> 2010-07-16  Steve Ellcey  <sje@cup.hp.com>
>
>        PR middle-end/44878
>        * tree.c (needs_to_live_in_memory):  Remove DECL_BY_REFERENCE check.
>
>
> Index: tree.c
> ===================================================================
> --- tree.c      (revision 162239)
> +++ tree.c      (working copy)
> @@ -9741,7 +9741,6 @@ needs_to_live_in_memory (const_tree t)
>   return (TREE_ADDRESSABLE (t)
>          || is_global_var (t)
>          || (TREE_CODE (t) == RESULT_DECL
> -             && !DECL_BY_REFERENCE (t)
>              && aggregate_value_p (t, current_function_decl)));
>  }
>
>

Re: [Patch, middle-end] Fix PR 44878, IA64 build failure, partial inlining

[Steve Ellcey]

On Sun, 2010-07-18 at 20:11 +0200, Richard Guenther wrote:

> The problem might be that the needs_to_live_in_memory predicate ends up
> being used in incompatible contexts.  Which of the callers ends up
> making the difference to you?
> 
> Richard.

Using my small test case, all of the calls to needs_to_live_in_memory
are coming from is_gimple_reg and all of the calls where I return a
different value after my patch are for the same return value reference.

I think the more likely incompatibility is in the use of
targetm.addr_space.pointer_mode (ptr_mode/SImode for me) and
targetm.addr_space.address_mode (Pmode/DImode) combined with the fact
that IA64 is one of the few platforms to define both PROMOTE_MODE
and POINTERS_EXTEND_UNSIGNED.  But I have no idea where the
incompatibility actual is.

I do find it interesting that promote_mode in explow.c doesn't call the
PROMOTE_MODE macro for REFERENCE_TYPE or POINTER_TYPE but just returns
targetm.addr_space.address_mode for those types.  I am not sure if that
is right or wrong but I find it a bit odd.

Steve Ellcey
sje@cup.hp.com

Re: [Patch, middle-end] Fix PR 44878, IA64 build failure, partial inlining

[Steve Ellcey]

Here is more analysis of where I think we might be going wrong.  The basic problem is that we get into
expand_value_return with val equal to:

(subreg/s/v:SI (reg/f:DI 341 [ <retval>+-4 ]) 4)

and a return_reg of:

(reg/f:DI 341 [ <retval>+-4 ])

The problem, of course, is the mode mismatch.  Since we are returning a reference and promote_mode explicitly
promotes references to Pmode (DImode) I think both of these should be DImode (and that is what they were before
the partial inline checkin that broke things).

So I looked at where val was set and I get to expand_expr_real_1 with an SSA_NAME and an rtx of:

(reg/f:DI 341 [ <retval>+-4 ])

which is good.  But then we execute 'goto expand_decl_rtl' and it is there that we convert our DImode register
to a SImode subreg.  We have:

 8472       /* If the mode of DECL_RTL does not match that of the decl, it
 8473          must be a promoted value.  We return a SUBREG of the wanted mode,
 8474          but mark it so that we know that it was already extended.  */
 8475       if (REG_P (decl_rtl) && GET_MODE (decl_rtl) != DECL_MODE (exp))
 8476         {
 8477           enum machine_mode pmode;
 8478 
 8479           /* Get the signedness to be used for this variable.  Ensure we g      et
 8480              the same mode we got when the variable was declared.  */
 8481           if (code == SSA_NAME
 8482               && (g = SSA_NAME_DEF_STMT (ssa_name))
 8483               && gimple_code (g) == GIMPLE_CALL) 
 8484             pmode = promote_function_mode (type, mode, &unsignedp,
 8485                                            TREE_TYPE
 8486                                            (TREE_TYPE (gimple_call_fn (g))      ),
 8487                                            2);
 8488           else 
 8489             pmode = promote_decl_mode (exp, &unsignedp);
 8490           gcc_assert (GET_MODE (decl_rtl) == pmode);
 8491 
 8492           temp = gen_lowpart_SUBREG (mode, decl_rtl);
 8493           SUBREG_PROMOTED_VAR_P (temp) = 1;
 8494           SUBREG_PROMOTED_UNSIGNED_SET (temp, unsignedp);
 8495           return temp;

decl_rtl is:

(reg/f:DI 341 [ <retval>+-4 ])

and exp is:

 <result_decl 65876460 D.1760
    type <reference_type 65888780
        type <record_type 6587aea0 e addressable needs-constructing type_1 type_5 BLK
            size <integer_cst 657d17a0 constant 8>
            unit size <integer_cst 657d17c0 constant 1>
            align 8 symtab 0 alias set -1 canonical type 6587aea0 fields <type_decl 65885070 e>
            full-name "class e"
            needs-constructor X(constX&) this=(X&) n_parents=0 use_template=0 interface-unknown
            pointer_to_this <pointer_type 65888060> reference_to_this <reference_type 65888780> chain <type_decl 65885000 e>>
        public unsigned SI
        size <integer_cst 657d1960 constant 32>
        unit size <integer_cst 657d1700 constant 4>
        align 32 symtab 0 alias set -1 canonical type 65888780>
   used unsigned ignored SI passed-by-reference file x.cc line 9 col 7 size <integer_cst 657d1960 32> unit size <integer_cst 657d1700 4>
    align 32 context <function_decl 6587bf00 foo>
    (reg/f:DI 341 [ <retval>+-4 ])>

The modes don't match and so we enter the if statement and generate a subreg of r341:

(subreg/s/v:SI (reg/f:DI 341 [ <retval>+-4 ]) 4)

and this results in the problem in expand_value_return.


Is the answer simply that we shouldn't enter this
if statement with a result_decl?

Steve Ellcey
sje@cup.hp.com

Re: [Patch, middle-end] Fix PR 44878, IA64 build failure, partial inlining

[Richard Guenther]

On Tue, Jul 20, 2010 at 10:41 PM, Steve Ellcey <sje@cup.hp.com> wrote:
> Here is more analysis of where I think we might be going wrong.  The basic problem is that we get into
> expand_value_return with val equal to:
>
> (subreg/s/v:SI (reg/f:DI 341 [ <retval>+-4 ]) 4)
>
> and a return_reg of:
>
> (reg/f:DI 341 [ <retval>+-4 ])
>
> The problem, of course, is the mode mismatch.  Since we are returning a reference and promote_mode explicitly
> promotes references to Pmode (DImode) I think both of these should be DImode (and that is what they were before
> the partial inline checkin that broke things).
>
> So I looked at where val was set and I get to expand_expr_real_1 with an SSA_NAME and an rtx of:
>
> (reg/f:DI 341 [ <retval>+-4 ])
>
> which is good.  But then we execute 'goto expand_decl_rtl' and it is there that we convert our DImode register
> to a SImode subreg.  We have:
>
>  8472       /* If the mode of DECL_RTL does not match that of the decl, it
>  8473          must be a promoted value.  We return a SUBREG of the wanted mode,
>  8474          but mark it so that we know that it was already extended.  */
>  8475       if (REG_P (decl_rtl) && GET_MODE (decl_rtl) != DECL_MODE (exp))
>  8476         {
>  8477           enum machine_mode pmode;
>  8478
>  8479           /* Get the signedness to be used for this variable.  Ensure we g      et
>  8480              the same mode we got when the variable was declared.  */
>  8481           if (code == SSA_NAME
>  8482               && (g = SSA_NAME_DEF_STMT (ssa_name))
>  8483               && gimple_code (g) == GIMPLE_CALL)
>  8484             pmode = promote_function_mode (type, mode, &unsignedp,
>  8485                                            TREE_TYPE
>  8486                                            (TREE_TYPE (gimple_call_fn (g))      ),
>  8487                                            2);
>  8488           else
>  8489             pmode = promote_decl_mode (exp, &unsignedp);
>  8490           gcc_assert (GET_MODE (decl_rtl) == pmode);
>  8491
>  8492           temp = gen_lowpart_SUBREG (mode, decl_rtl);
>  8493           SUBREG_PROMOTED_VAR_P (temp) = 1;
>  8494           SUBREG_PROMOTED_UNSIGNED_SET (temp, unsignedp);
>  8495           return temp;
>
> decl_rtl is:
>
> (reg/f:DI 341 [ <retval>+-4 ])
>
> and exp is:
>
>  <result_decl 65876460 D.1760
>    type <reference_type 65888780
>        type <record_type 6587aea0 e addressable needs-constructing type_1 type_5 BLK
>            size <integer_cst 657d17a0 constant 8>
>            unit size <integer_cst 657d17c0 constant 1>
>            align 8 symtab 0 alias set -1 canonical type 6587aea0 fields <type_decl 65885070 e>
>            full-name "class e"
>            needs-constructor X(constX&) this=(X&) n_parents=0 use_template=0 interface-unknown
>            pointer_to_this <pointer_type 65888060> reference_to_this <reference_type 65888780> chain <type_decl 65885000 e>>
>        public unsigned SI
>        size <integer_cst 657d1960 constant 32>
>        unit size <integer_cst 657d1700 constant 4>
>        align 32 symtab 0 alias set -1 canonical type 65888780>
>   used unsigned ignored SI passed-by-reference file x.cc line 9 col 7 size <integer_cst 657d1960 32> unit size <integer_cst 657d1700 4>
>    align 32 context <function_decl 6587bf00 foo>
>    (reg/f:DI 341 [ <retval>+-4 ])>
>
> The modes don't match and so we enter the if statement and generate a subreg of r341:
>
> (subreg/s/v:SI (reg/f:DI 341 [ <retval>+-4 ]) 4)
>
> and this results in the problem in expand_value_return.
>
>
> Is the answer simply that we shouldn't enter this
> if statement with a result_decl?

The question is where and why does (reg/f:DI 341 [ <retval>+-4 ]) get
DI mode from.  The fix would be to simply handle it the same in the
above
code, avoiding the subreg (<retval>+-4 shows me that this indeed
is a promoted subreg - maybe adding

                else if (code == RESULT_DECL)
 8484             pmode = promote_function_mode (type, mode, &unsignedp,
 8486                                            TREE_TYPE
(current_function_decl),
 8487                                            2);

helps?

Richard.


> Steve Ellcey
> sje@cup.hp.com
>
>

Re: [Patch, middle-end] Fix PR 44878, IA64 build failure, partial inlining

[Steve Ellcey]

Here is my latest patch to fix this problem.  I think the issue involves
the for_return argument to promote_function_mode.  The comments for
promote_function_mode simply say that this argument is non-zero if we
are promoting a return value (and not an argument).

But the routine calls targetm.calls.promote_function_mode and if that 
is set to default_promote_function_mode, the code checks for 
(for_return == 2) and calls promote_mode if it is.  This behaviour
is documented in tm.texi:

        @var{for_return} allows to distinguish the promotion of
        arguments and return values.  If it is @code{1}, a return value
        is being promoted and @code{TARGET_FUNCTION_VALUE} must perform
        the same promotions done here.  If it is @code{2}, the returned
        mode should be that of the register in which an incoming
        parameter is copied, or the outgoing result is computed; then
        the hook should return the same mode as @code{promote_mode},
        though the signedness may be different.
        
Given this, it seems that expand_value_return should check for a
reference return and call promote_function_mode with 2 instead of 1
for that case.  This patch allows me to bootstrap on IA64 and the
testing seems to be going OK.  Assuming it works, is this patch OK to
checkin?

Steve Ellcey
sje@cup.hp.com

Index: stmt.c
===================================================================
--- stmt.c      (revision 162360)
+++ stmt.c      (working copy)
@@ -1595,8 +1595,11 @@ expand_value_return (rtx val)
       tree type = TREE_TYPE (decl);
       int unsignedp = TYPE_UNSIGNED (type);
       enum machine_mode old_mode = DECL_MODE (decl);
-      enum machine_mode mode = promote_function_mode (type, old_mode,
-                                                     &unsignedp, funtype, 1);
+      enum machine_mode mode;
+      if (DECL_BY_REFERENCE (decl))
+        mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 2);
+      else
+        mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 1);
 
       if (mode != old_mode)
        val = convert_modes (mode, old_mode, val, unsignedp);

Re: [Patch, middle-end] Fix PR 44878, IA64 build failure, partial inlining

[Richard Guenther]

On Wed, Jul 21, 2010 at 11:00 PM, Steve Ellcey <sje@cup.hp.com> wrote:
>
> Here is my latest patch to fix this problem.  I think the issue involves
> the for_return argument to promote_function_mode.  The comments for
> promote_function_mode simply say that this argument is non-zero if we
> are promoting a return value (and not an argument).
>
> But the routine calls targetm.calls.promote_function_mode and if that
> is set to default_promote_function_mode, the code checks for
> (for_return == 2) and calls promote_mode if it is.  This behaviour
> is documented in tm.texi:
>
>        @var{for_return} allows to distinguish the promotion of
>        arguments and return values.  If it is @code{1}, a return value
>        is being promoted and @code{TARGET_FUNCTION_VALUE} must perform
>        the same promotions done here.  If it is @code{2}, the returned
>        mode should be that of the register in which an incoming
>        parameter is copied, or the outgoing result is computed; then
>        the hook should return the same mode as @code{promote_mode},
>        though the signedness may be different.
>
> Given this, it seems that expand_value_return should check for a
> reference return and call promote_function_mode with 2 instead of 1
> for that case.  This patch allows me to bootstrap on IA64 and the
> testing seems to be going OK.  Assuming it works, is this patch OK to
> checkin?

Hm, it sounds reasonable.  But I'd like someone more familiar with
the code review the patch.

Thanks,
Richard.

> Steve Ellcey
> sje@cup.hp.com
>
> Index: stmt.c
> ===================================================================
> --- stmt.c      (revision 162360)
> +++ stmt.c      (working copy)
> @@ -1595,8 +1595,11 @@ expand_value_return (rtx val)
>       tree type = TREE_TYPE (decl);
>       int unsignedp = TYPE_UNSIGNED (type);
>       enum machine_mode old_mode = DECL_MODE (decl);
> -      enum machine_mode mode = promote_function_mode (type, old_mode,
> -                                                     &unsignedp, funtype, 1);
> +      enum machine_mode mode;
> +      if (DECL_BY_REFERENCE (decl))
> +        mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 2);
> +      else
> +        mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 1);
>
>       if (mode != old_mode)
>        val = convert_modes (mode, old_mode, val, unsignedp);
>
>

Re: [Patch, middle-end] Fix PR 44878, IA64 build failure, partial inlining

[Ian Lance Taylor]

Richard Guenther <richard.guenther@gmail.com> writes:

> On Wed, Jul 21, 2010 at 11:00 PM, Steve Ellcey <sje@cup.hp.com> wrote:
>>
>> Here is my latest patch to fix this problem.  I think the issue involves
>> the for_return argument to promote_function_mode.  The comments for
>> promote_function_mode simply say that this argument is non-zero if we
>> are promoting a return value (and not an argument).
>>
>> But the routine calls targetm.calls.promote_function_mode and if that
>> is set to default_promote_function_mode, the code checks for
>> (for_return == 2) and calls promote_mode if it is.  This behaviour
>> is documented in tm.texi:
>>
>>        @var{for_return} allows to distinguish the promotion of
>>        arguments and return values.  If it is @code{1}, a return value
>>        is being promoted and @code{TARGET_FUNCTION_VALUE} must perform
>>        the same promotions done here.  If it is @code{2}, the returned
>>        mode should be that of the register in which an incoming
>>        parameter is copied, or the outgoing result is computed; then
>>        the hook should return the same mode as @code{promote_mode},
>>        though the signedness may be different.
>>
>> Given this, it seems that expand_value_return should check for a
>> reference return and call promote_function_mode with 2 instead of 1
>> for that case.  This patch allows me to bootstrap on IA64 and the
>> testing seems to be going OK.  Assuming it works, is this patch OK to
>> checkin?
>
> Hm, it sounds reasonable.  But I'd like someone more familiar with
> the code review the patch.

This looks OK to me.

Thanks.

Ian

>> ===================================================================
>> --- stmt.c      (revision 162360)
>> +++ stmt.c      (working copy)
>> @@ -1595,8 +1595,11 @@ expand_value_return (rtx val)
>>       tree type = TREE_TYPE (decl);
>>       int unsignedp = TYPE_UNSIGNED (type);
>>       enum machine_mode old_mode = DECL_MODE (decl);
>> -      enum machine_mode mode = promote_function_mode (type, old_mode,
>> -                                                     &unsignedp, funtype, 1);
>> +      enum machine_mode mode;
>> +      if (DECL_BY_REFERENCE (decl))
>> +        mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 2);
>> +      else
>> +        mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 1);
>>
>>       if (mode != old_mode)
>>        val = convert_modes (mode, old_mode, val, unsignedp);
>>
>>

Re: [Patch, middle-end] Fix PR 44878, IA64 build failure, partial inlining

[Richard Henderson]

On 07/21/2010 02:00 PM, Steve Ellcey wrote:
> +      enum machine_mode mode;
> +      if (DECL_BY_REFERENCE (decl))
> +        mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 2);
> +      else
> +        mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 1);

I think the logic is good.  I would merely combine the two calls:

  mode = promote_function_mode (type, old_mode, &unsignedp, funtype,
				(DECL_BY_REFERENCE (decl) ? 2 : 1));


r~

Re: [Patch, middle-end] Fix PR 44878, IA64 build failure, partial inlining

[Steve Ellcey]

On Thu, 2010-07-22 at 11:28 -0700, Richard Henderson wrote:
> On 07/21/2010 02:00 PM, Steve Ellcey wrote:
> > +      enum machine_mode mode;
> > +      if (DECL_BY_REFERENCE (decl))
> > +        mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 2);
> > +      else
> > +        mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 1);
> 
> I think the logic is good.  I would merely combine the two calls:
> 
>   mode = promote_function_mode (type, old_mode, &unsignedp, funtype,
> 				(DECL_BY_REFERENCE (decl) ? 2 : 1));
> 
> 
> r~

I just checked in the original version a minute ago.  Should I change
it?

Steve Ellcey
sje@cup.hp.com

Re: [Patch, middle-end] Fix PR 44878, IA64 build failure, partial inlining

[Richard Henderson]

On 07/22/2010 11:31 AM, Steve Ellcey wrote:
> I just checked in the original version a minute ago.  Should I change
> it?

I suppose not.

Better of course would be to have a pass that performs this
optimization automatically of course.  ;-)


r~