| Message ID | 201007162023.o6GKNDN19230@lucas.cup.hp.com |
|---|---|
| State | New |
| Headers | show |
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))); > } > >
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
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
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 > >
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))); }
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.