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

login
register
mail settings
Submitter Steve Ellcey
Date July 16, 2010, 8:23 p.m.
Message ID <201007162023.o6GKNDN19230@lucas.cup.hp.com>
Download mbox | patch
Permalink /patch/59124/
State New
Headers show

Comments

Steve Ellcey - July 16, 2010, 8:23 p.m.
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.
Richard Guenther - July 18, 2010, 6:11 p.m.
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)));
>  }
>
>
Steve Ellcey - July 19, 2010, 6:09 p.m.
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
Steve Ellcey - July 20, 2010, 8:41 p.m.
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
Richard Guenther - July 21, 2010, 8:32 a.m.
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
>
>

Patch

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)));
 }