| Message ID | 20100704224222.GB29130@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
On Mon, 5 Jul 2010, Jan Hubicka wrote: > Hi, > the testcase in PR (Richard is running minimizing on it) produce ICE because > ipa-split produce invalid gimple > *<retval> = funcall.part (); > where <retval> is not gimple register. It is function returning by reference > so we actually must write to retval directly for correctness. <retval> is not > gimple register because needs_to_live_in_memory returns true after calling > aggregate_value_p. > > This is confused. For functions returning by reference retval is pointer and > as such it does not need to live in memory. aggregate_value_p is about return > value itself, that is *<retval> that lives in memory. > > So this patch changes behaviour of needs_to_live_in_memory that has some consequences. > > 1) We get confused wanrings since tree-ssa-uninit.c considers retvals not initilized > at function entry. This is not valid for DECL_BY_REFERENCE> > > 2) tree-inline produces bogus &tmp_1=&tmp_1 for return statement, since it no longer > recognizes return value wrapped in SSA name. > > 3) Alias analysis don't know that <retval>.filed stores escape and thus must > be preserved > > 4) Statement verifier does type checking when return statement is not retval. > It should check DECL_BY_REFERENCE, but Richard plans to submit patch for it. > So I just patched around strengthening the check for SSA name of DECL_BY_REFERENCE> > > Bootstrapped/regtested x86_64 and tested on Mozilla build. OK? > > Honza > > PR middle-end/44813 > * tree-ssa-uninit.c (ssa_undefined_value_p): Retval_expr is initialized > for functions returning by reference. > * tree.c (needs_to_live_in_memory): Retval for functions returning by > reference does not need to live in memory. > * tree-inline.c (remap_gimple_stmt): Do not produce assignment > for functions returning by reference. > * tree-ssa-structalias.c (find_what_p_points_to): Look into result decls > same was as into parm decls. > * tree-cfg.c (verify_gimple_return): Expect RESULT_DECL to have SSA name. > Index: tree-ssa-uninit.c > =================================================================== > --- tree-ssa-uninit.c (revision 161774) > +++ tree-ssa-uninit.c (working copy) > @@ -92,6 +92,12 @@ ssa_undefined_value_p (tree t) > if (TREE_CODE (var) == PARM_DECL) > return false; > > + /* When returning by reference the return address is actually a hidden > + parameter. */ > + if (TREE_CODE (SSA_NAME_VAR (t)) == RESULT_DECL > + && DECL_BY_REFERENCE (SSA_NAME_VAR (t))) > + return false; > + > /* Hard register variables get their initial value from the ether. */ > if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var)) > return false; This change is ok. > Index: tree.c > =================================================================== > --- tree.c (revision 161774) > +++ tree.c (working copy) > @@ -9750,6 +9750,7 @@ 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))); > } Likewise. > Index: tree-inline.c > =================================================================== > --- tree-inline.c (revision 161774) > +++ tree-inline.c (working copy) > @@ -1236,7 +1237,11 @@ remap_gimple_stmt (gimple stmt, copy_bod > If RETVAL is just the result decl, the result decl has > already been set (e.g. a recent "foo (&result_decl, ...)"); > just toss the entire GIMPLE_RETURN. */ > - if (retval && TREE_CODE (retval) != RESULT_DECL) > + if (retval > + && (TREE_CODE (retval) != RESULT_DECL > + && (TREE_CODE (retval) != SSA_NAME > + || !SSA_NAME_IS_DEFAULT_DEF (retval) > + || TREE_CODE (SSA_NAME_VAR (retval)) != RESULT_DECL))) We should never see a defintion of a RESULT_DECL SSA name for DECL_BY_REFERENCE RESULT_DECLs (that would be a bug - we should add verification to the SSA verifier, can you do add that?). So the || !SSA_NAME_IS_DEFAULT_DEF (retval) can be dropped here. This patch is ok with that change together with an addition to the SSA verifier. > { > copy = gimple_build_assign (id->retvar, retval); > /* id->retvar is already substituted. Skip it on later remapping. */ > Index: tree-ssa-structalias.c > =================================================================== > --- tree-ssa-structalias.c (revision 161774) > +++ tree-ssa-structalias.c (working copy) > @@ -5751,7 +5751,8 @@ find_what_p_points_to (tree p) > /* For parameters, get at the points-to set for the actual parm > decl. */ > if (TREE_CODE (p) == SSA_NAME > - && TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL > + && (TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL > + || TREE_CODE (SSA_NAME_VAR (p)) == RESULT_DECL) > && SSA_NAME_IS_DEFAULT_DEF (p)) > lookup_p = SSA_NAME_VAR (p); That doesn't look correct on its own. In fact get_constraint_for_ssa_var needs a similar adjustment, likewise for consistency (probably doesn't happen here though) get_fi_for_callee. The patch is ok with those changes. > Index: tree-cfg.c > =================================================================== > --- tree-cfg.c (revision 161774) > +++ tree-cfg.c (working copy) > @@ -3818,8 +3818,7 @@ verify_gimple_return (gimple stmt) > if (op == NULL) > return false; > > - if (!is_gimple_val (op) > - && TREE_CODE (op) != RESULT_DECL) > + if (!is_gimple_val (op) && TREE_CODE (op) != RESULT_DECL) > { > error ("invalid operand in return statement"); > debug_generic_stmt (op); > @@ -3829,7 +3828,10 @@ verify_gimple_return (gimple stmt) > if (!useless_type_conversion_p (restype, TREE_TYPE (op)) > /* ??? With C++ we can have the situation that the result > decl is a reference type while the return type is an aggregate. */ > - && !(TREE_CODE (op) == RESULT_DECL > + && !((TREE_CODE (op) == RESULT_DECL > + || (TREE_CODE (op) == SSA_NAME > + && SSA_NAME_IS_DEFAULT_DEF (op) > + && TREE_CODE (SSA_NAME_VAR (op)) == RESULT_DECL)) > && TREE_CODE (TREE_TYPE (op)) == REFERENCE_TYPE > && useless_type_conversion_p (restype, TREE_TYPE (TREE_TYPE (op))))) > { I have committed a change similar to this already. Thanks, Richard.
> > Index: tree-inline.c > > =================================================================== > > --- tree-inline.c (revision 161774) > > +++ tree-inline.c (working copy) > > @@ -1236,7 +1237,11 @@ remap_gimple_stmt (gimple stmt, copy_bod > > If RETVAL is just the result decl, the result decl has > > already been set (e.g. a recent "foo (&result_decl, ...)"); > > just toss the entire GIMPLE_RETURN. */ > > - if (retval && TREE_CODE (retval) != RESULT_DECL) > > + if (retval > > + && (TREE_CODE (retval) != RESULT_DECL > > + && (TREE_CODE (retval) != SSA_NAME > > + || !SSA_NAME_IS_DEFAULT_DEF (retval) > > + || TREE_CODE (SSA_NAME_VAR (retval)) != RESULT_DECL))) > > We should never see a defintion of a RESULT_DECL SSA name for > DECL_BY_REFERENCE RESULT_DECLs (that would > be a bug - we should add verification to the SSA verifier, can you > do add that?). So the || !SSA_NAME_IS_DEFAULT_DEF (retval) can be > dropped here. OK. I just wanted to play safe. > > This patch is ok with that change together with an addition to the > SSA verifier. > > > { > > copy = gimple_build_assign (id->retvar, retval); > > /* id->retvar is already substituted. Skip it on later remapping. */ > > Index: tree-ssa-structalias.c > > =================================================================== > > --- tree-ssa-structalias.c (revision 161774) > > +++ tree-ssa-structalias.c (working copy) > > @@ -5751,7 +5751,8 @@ find_what_p_points_to (tree p) > > /* For parameters, get at the points-to set for the actual parm > > decl. */ > > if (TREE_CODE (p) == SSA_NAME > > - && TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL > > + && (TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL > > + || TREE_CODE (SSA_NAME_VAR (p)) == RESULT_DECL) > > && SSA_NAME_IS_DEFAULT_DEF (p)) > > lookup_p = SSA_NAME_VAR (p); > > That doesn't look correct on its own. In fact get_constraint_for_ssa_var > needs a similar adjustment, likewise for consistency (probably doesn't > happen here though) get_fi_for_callee. Will try, thanks! Honza
Hi,
here is updated patch, with Richard's reduced testcase that solves some furhter
problems.
In particular adding the tester down that ipa-split produce new SSA name
for return value and sets as definining statement the call (that is wrong
for DECL_BY_REFERENCE)> I also cleaned up the code a bit there.
Also ipa-ssa-ccp considers RESULT_DECL as undefined that leads to misoptimization
after fixing the first problem on libstdc++ vector testcase.
Bootstrapped/regtested x86_64-linux, OK?
* tree-ssa-uninit.c (ssa_undefined_value_p): Result decl is defined
for functions passed by reference.
* tree.c (needs_to_live_in_memory): RESULT_DECL don't need to live
in memory when passed by reference.
* tree-ssa-ccp.c (get_default_value): Only VAR_DECL is undefined at
beggining.
* ipa-split.c (split_function): Cleanup way return value is passed;
handle SSA DECL_BY_REFERENCE retvals.
* tree-ssa.c (verify_def): Verify that RESULT_DECL is read only when
DECL_BY_REFERENCE is set.
* tree-inline.c (remap_gimple_stmt): Handle SSA DECL_BY_REFERENCE
returns.
* tree-ssa-structalias.c (get_constraint_for_ssa_var, get_fi_for_callee,
find_what_p_points_to): Handle RESULT_DECL.
* testsuite/g++.dg/torture/pr44813.C: New testcase.
Index: tree-ssa-uninit.c
===================================================================
*** tree-ssa-uninit.c (revision 161826)
--- tree-ssa-uninit.c (working copy)
*************** ssa_undefined_value_p (tree t)
*** 92,97 ****
--- 92,103 ----
if (TREE_CODE (var) == PARM_DECL)
return false;
+ /* When returning by reference the return address is actually a hidden
+ parameter. */
+ if (TREE_CODE (SSA_NAME_VAR (t)) == RESULT_DECL
+ && DECL_BY_REFERENCE (SSA_NAME_VAR (t)))
+ return false;
+
/* Hard register variables get their initial value from the ether. */
if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
return false;
Index: tree.c
===================================================================
*** tree.c (revision 161826)
--- tree.c (working copy)
*************** needs_to_live_in_memory (const_tree t)
*** 9750,9755 ****
--- 9750,9756 ----
return (TREE_ADDRESSABLE (t)
|| is_global_var (t)
|| (TREE_CODE (t) == RESULT_DECL
+ && !DECL_BY_REFERENCE (t)
&& aggregate_value_p (t, current_function_decl)));
}
Index: testsuite/g++.dg/torture/pr44813.C
===================================================================
*** testsuite/g++.dg/torture/pr44813.C (revision 0)
--- testsuite/g++.dg/torture/pr44813.C (revision 0)
***************
*** 0 ****
--- 1,60 ----
+ typedef unsigned int PRUint32;
+ typedef int PRInt32;
+ typedef unsigned long PRUint64;
+ typedef int PRIntn;
+ typedef PRIntn PRBool;
+ struct nsRect {
+ nsRect(const nsRect& aRect) { }
+ };
+ enum nsCompatibility { eCompatibility_NavQuirks = 3 };
+ class gfxContext;
+ typedef PRUint64 nsFrameState;
+ class nsPresContext {
+ public:
+ nsCompatibility CompatibilityMode() const { }
+ };
+ class nsStyleContext {
+ public:
+ PRBool HasTextDecorations() const;
+ };
+ class nsIFrame {
+ public:
+ nsPresContext* PresContext() const;
+ nsStyleContext* GetStyleContext() const;
+ nsFrameState GetStateBits() const;
+ nsRect GetOverflowRect() const;
+ };
+ class nsFrame : public nsIFrame { };
+ class nsLineList_iterator { };
+ class nsLineList {
+ public:
+ typedef nsLineList_iterator iterator;
+ };
+ class gfxSkipCharsIterator { };
+ class gfxTextRun {
+ public:
+ class PropertyProvider { };
+ };
+ class nsTextFrame : public nsFrame
+ {
+ virtual nsRect ComputeTightBounds(gfxContext* aContext) const;
+ gfxSkipCharsIterator EnsureTextRun(gfxContext* aReferenceContext = 0L,
+ nsIFrame* aLineContainer = 0L,
+ const nsLineList::iterator* aLine = 0L,
+ PRUint32* aFlowEndInTextRun = 0L);
+ };
+ class PropertyProvider : public gfxTextRun::PropertyProvider
+ {
+ public:
+ PropertyProvider(nsTextFrame* aFrame, const gfxSkipCharsIterator& aStart);
+ PRInt32 mLength[64];
+ };
+ nsRect nsTextFrame::ComputeTightBounds(gfxContext* aContext) const
+ {
+ if ((GetStyleContext()->HasTextDecorations()
+ && eCompatibility_NavQuirks == PresContext()->CompatibilityMode())
+ || (GetStateBits() & (nsFrameState(1) << (23))))
+ return GetOverflowRect();
+ gfxSkipCharsIterator iter = const_cast<nsTextFrame*>(this)->EnsureTextRun();
+ PropertyProvider provider(const_cast<nsTextFrame*>(this), iter);
+ }
Index: tree-ssa-ccp.c
===================================================================
*** tree-ssa-ccp.c (revision 161826)
--- tree-ssa-ccp.c (working copy)
*************** get_default_value (tree var)
*** 300,306 ****
before being initialized. If VAR is a local variable, we
can assume initially that it is UNDEFINED, otherwise we must
consider it VARYING. */
! if (is_gimple_reg (sym) && TREE_CODE (sym) != PARM_DECL)
val.lattice_val = UNDEFINED;
else
val.lattice_val = VARYING;
--- 300,307 ----
before being initialized. If VAR is a local variable, we
can assume initially that it is UNDEFINED, otherwise we must
consider it VARYING. */
! if (is_gimple_reg (sym)
! && TREE_CODE (sym) == VAR_DECL)
val.lattice_val = UNDEFINED;
else
val.lattice_val = VARYING;
Index: ipa-split.c
===================================================================
*** ipa-split.c (revision 161826)
--- ipa-split.c (working copy)
*************** split_function (struct split_point *spli
*** 967,997 ****
return_bb == EXIT_BLOCK_PTR ? 0 : EDGE_FALLTHRU);
e->count = call_bb->count;
e->probability = REG_BR_PROB_BASE;
if (return_bb != EXIT_BLOCK_PTR)
{
real_retval = retval = find_retval (return_bb);
if (real_retval
&& !is_gimple_min_invariant (retval)
&& (TREE_CODE (retval) != SSA_NAME
! || !SSA_NAME_IS_DEFAULT_DEF (retval)))
{
gimple_stmt_iterator psi;
! /* See if there is PHI defining return value. */
! for (psi = gsi_start_phis (return_bb);
! !gsi_end_p (psi); gsi_next (&psi))
! if (is_gimple_reg (gimple_phi_result (gsi_stmt (psi))))
! break;
!
! /* When we have PHI, update PHI. When there is no PHI,
! update the return statement itself. */
! if (TREE_CODE (retval) == SSA_NAME)
{
retval = make_ssa_name (SSA_NAME_VAR (retval), call);
if (TREE_CODE (retval) == SSA_NAME
&& !gsi_end_p (psi))
add_phi_arg (gsi_stmt (psi), retval, e, UNKNOWN_LOCATION);
! else if (TREE_CODE (retval) == SSA_NAME)
{
gimple_stmt_iterator bsi;
for (bsi = gsi_start_bb (return_bb); !gsi_end_p (bsi);
--- 967,1013 ----
return_bb == EXIT_BLOCK_PTR ? 0 : EDGE_FALLTHRU);
e->count = call_bb->count;
e->probability = REG_BR_PROB_BASE;
+
+ /* If there is return basic block, see what value we need to store
+ return value into and put call just before it. */
if (return_bb != EXIT_BLOCK_PTR)
{
real_retval = retval = find_retval (return_bb);
+
+ /* See if return value is computed by split part;
+ function might just return its argument, invariant or undefined
+ value. In this case we don't need to do any updating. */
if (real_retval
&& !is_gimple_min_invariant (retval)
&& (TREE_CODE (retval) != SSA_NAME
! || (!SSA_NAME_IS_DEFAULT_DEF (retval)
! || DECL_BY_REFERENCE
! (DECL_RESULT (current_function_decl)))))
{
gimple_stmt_iterator psi;
! /* See if we need new SSA_NAME for the result.
! When DECL_BY_REFERENCE is true, retval is actually pointer to
! return value and it is constant in whole function. */
! if (TREE_CODE (retval) == SSA_NAME
! && !DECL_BY_REFERENCE (DECL_RESULT (current_function_decl)))
{
retval = make_ssa_name (SSA_NAME_VAR (retval), call);
+
+ /* See if there is PHI defining return value. */
+ for (psi = gsi_start_phis (return_bb);
+ !gsi_end_p (psi); gsi_next (&psi))
+ if (is_gimple_reg (gimple_phi_result (gsi_stmt (psi))))
+ break;
+
+ /* When there is PHI, just update its value. */
if (TREE_CODE (retval) == SSA_NAME
&& !gsi_end_p (psi))
add_phi_arg (gsi_stmt (psi), retval, e, UNKNOWN_LOCATION);
! /* Otherwise update the return BB itself.
! find_return_bb allows at most one assignment to return value,
! so update first statement. */
! else
{
gimple_stmt_iterator bsi;
for (bsi = gsi_start_bb (return_bb); !gsi_end_p (bsi);
*************** split_function (struct split_point *spli
*** 1016,1021 ****
--- 1032,1040 ----
}
gsi_insert_after (&gsi, call, GSI_NEW_STMT);
}
+ /* We don't use return block (there is either no return in function or
+ multiple of them). So create new basic block with return statement.
+ */
else
{
gimple ret;
*************** split_function (struct split_point *spli
*** 1030,1036 ****
&& !DECL_BY_REFERENCE (retval))
retval = create_tmp_reg (TREE_TYPE (retval), NULL);
if (is_gimple_reg (retval))
! retval = make_ssa_name (retval, call);
if (DECL_BY_REFERENCE (DECL_RESULT (current_function_decl)))
gimple_call_set_lhs (call, build_simple_mem_ref (retval));
else
--- 1049,1076 ----
&& !DECL_BY_REFERENCE (retval))
retval = create_tmp_reg (TREE_TYPE (retval), NULL);
if (is_gimple_reg (retval))
! {
! /* When returning by reference, there is only one SSA name
! assigned to RESULT_DECL (that is pointer to return value).
! Look it up or create new one if it is missing. */
! if (DECL_BY_REFERENCE (retval))
! {
! tree retval_name;
! if ((retval_name = gimple_default_def (cfun, retval))
! != NULL)
! retval = retval_name;
! else
! {
! retval_name = make_ssa_name (retval,
! gimple_build_nop ());
! set_default_def (retval, retval_name);
! retval = retval_name;
! }
! }
! /* Otherwise produce new SSA name for return value. */
! else
! retval = make_ssa_name (retval, call);
! }
if (DECL_BY_REFERENCE (DECL_RESULT (current_function_decl)))
gimple_call_set_lhs (call, build_simple_mem_ref (retval));
else
Index: tree-ssa.c
===================================================================
*** tree-ssa.c (revision 161826)
--- tree-ssa.c (working copy)
*************** verify_def (basic_block bb, basic_block
*** 638,643 ****
--- 638,650 ----
if (verify_ssa_name (ssa_name, is_virtual))
goto err;
+ if (TREE_CODE (SSA_NAME_VAR (ssa_name)) == RESULT_DECL
+ && DECL_BY_REFERENCE (SSA_NAME_VAR (ssa_name)))
+ {
+ error ("RESULT_DECL should be read only when DECL_BY_REFERENCE is set.");
+ goto err;
+ }
+
if (definition_block[SSA_NAME_VERSION (ssa_name)])
{
error ("SSA_NAME created in two different blocks %i and %i",
Index: tree-inline.c
===================================================================
*** tree-inline.c (revision 161826)
--- tree-inline.c (working copy)
*************** remap_gimple_stmt (gimple stmt, copy_bod
*** 1236,1242 ****
If RETVAL is just the result decl, the result decl has
already been set (e.g. a recent "foo (&result_decl, ...)");
just toss the entire GIMPLE_RETURN. */
! if (retval && TREE_CODE (retval) != RESULT_DECL)
{
copy = gimple_build_assign (id->retvar, retval);
/* id->retvar is already substituted. Skip it on later remapping. */
--- 1237,1246 ----
If RETVAL is just the result decl, the result decl has
already been set (e.g. a recent "foo (&result_decl, ...)");
just toss the entire GIMPLE_RETURN. */
! if (retval
! && (TREE_CODE (retval) != RESULT_DECL
! && (TREE_CODE (retval) != SSA_NAME
! || TREE_CODE (SSA_NAME_VAR (retval)) != RESULT_DECL)))
{
copy = gimple_build_assign (id->retvar, retval);
/* id->retvar is already substituted. Skip it on later remapping. */
Index: tree-ssa-structalias.c
===================================================================
*** tree-ssa-structalias.c (revision 161826)
--- tree-ssa-structalias.c (working copy)
*************** get_constraint_for_ssa_var (tree t, VEC(
*** 2836,2842 ****
/* For parameters, get at the points-to set for the actual parm
decl. */
if (TREE_CODE (t) == SSA_NAME
! && TREE_CODE (SSA_NAME_VAR (t)) == PARM_DECL
&& SSA_NAME_IS_DEFAULT_DEF (t))
{
get_constraint_for_ssa_var (SSA_NAME_VAR (t), results, address_p);
--- 2836,2844 ----
/* For parameters, get at the points-to set for the actual parm
decl. */
if (TREE_CODE (t) == SSA_NAME
! && (TREE_CODE (SSA_NAME_VAR (t)) == PARM_DECL
! || (TREE_CODE (SSA_NAME_VAR (t)) == RESULT_DECL
! && DECL_BY_REFERENCE (SSA_NAME_VAR (t))))
&& SSA_NAME_IS_DEFAULT_DEF (t))
{
get_constraint_for_ssa_var (SSA_NAME_VAR (t), results, address_p);
*************** get_fi_for_callee (gimple call)
*** 3982,3988 ****
if (TREE_CODE (decl) == SSA_NAME)
{
if (TREE_CODE (decl) == SSA_NAME
! && TREE_CODE (SSA_NAME_VAR (decl)) == PARM_DECL
&& SSA_NAME_IS_DEFAULT_DEF (decl))
decl = SSA_NAME_VAR (decl);
return get_vi_for_tree (decl);
--- 3984,3992 ----
if (TREE_CODE (decl) == SSA_NAME)
{
if (TREE_CODE (decl) == SSA_NAME
! && (TREE_CODE (SSA_NAME_VAR (decl)) == PARM_DECL
! || (TREE_CODE (SSA_NAME_VAR (decl)) == RESULT_DECL
! && DECL_BY_REFERENCE (SSA_NAME_VAR (decl))))
&& SSA_NAME_IS_DEFAULT_DEF (decl))
decl = SSA_NAME_VAR (decl);
return get_vi_for_tree (decl);
*************** find_what_p_points_to (tree p)
*** 5751,5757 ****
/* For parameters, get at the points-to set for the actual parm
decl. */
if (TREE_CODE (p) == SSA_NAME
! && TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL
&& SSA_NAME_IS_DEFAULT_DEF (p))
lookup_p = SSA_NAME_VAR (p);
--- 5755,5763 ----
/* For parameters, get at the points-to set for the actual parm
decl. */
if (TREE_CODE (p) == SSA_NAME
! && (TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL
! || (TREE_CODE (SSA_NAME_VAR (p)) == RESULT_DECL
! && DECL_BY_REFERENCE (SSA_NAME_VAR (p))))
&& SSA_NAME_IS_DEFAULT_DEF (p))
lookup_p = SSA_NAME_VAR (p);
On Mon, 5 Jul 2010, Jan Hubicka wrote: > Hi, > here is updated patch, with Richard's reduced testcase that solves some furhter > problems. > In particular adding the tester down that ipa-split produce new SSA name > for return value and sets as definining statement the call (that is wrong > for DECL_BY_REFERENCE)> I also cleaned up the code a bit there. > Also ipa-ssa-ccp considers RESULT_DECL as undefined that leads to misoptimization > after fixing the first problem on libstdc++ vector testcase. > > Bootstrapped/regtested x86_64-linux, OK? > > * tree-ssa-uninit.c (ssa_undefined_value_p): Result decl is defined > for functions passed by reference. > * tree.c (needs_to_live_in_memory): RESULT_DECL don't need to live > in memory when passed by reference. > * tree-ssa-ccp.c (get_default_value): Only VAR_DECL is undefined at > beggining. > * ipa-split.c (split_function): Cleanup way return value is passed; > handle SSA DECL_BY_REFERENCE retvals. > * tree-ssa.c (verify_def): Verify that RESULT_DECL is read only when > DECL_BY_REFERENCE is set. > * tree-inline.c (remap_gimple_stmt): Handle SSA DECL_BY_REFERENCE > returns. > * tree-ssa-structalias.c (get_constraint_for_ssa_var, get_fi_for_callee, > find_what_p_points_to): Handle RESULT_DECL. > > * testsuite/g++.dg/torture/pr44813.C: New testcase. > Index: tree-ssa-uninit.c > =================================================================== > *** tree-ssa-uninit.c (revision 161826) > --- tree-ssa-uninit.c (working copy) > *************** ssa_undefined_value_p (tree t) > *** 92,97 **** > --- 92,103 ---- > if (TREE_CODE (var) == PARM_DECL) > return false; > > + /* When returning by reference the return address is actually a hidden > + parameter. */ > + if (TREE_CODE (SSA_NAME_VAR (t)) == RESULT_DECL > + && DECL_BY_REFERENCE (SSA_NAME_VAR (t))) > + return false; > + > /* Hard register variables get their initial value from the ether. */ > if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var)) > return false; > Index: tree.c > =================================================================== > *** tree.c (revision 161826) > --- tree.c (working copy) > *************** needs_to_live_in_memory (const_tree t) > *** 9750,9755 **** > --- 9750,9756 ---- > return (TREE_ADDRESSABLE (t) > || is_global_var (t) > || (TREE_CODE (t) == RESULT_DECL > + && !DECL_BY_REFERENCE (t) > && aggregate_value_p (t, current_function_decl))); > } > > Index: testsuite/g++.dg/torture/pr44813.C > =================================================================== > *** testsuite/g++.dg/torture/pr44813.C (revision 0) > --- testsuite/g++.dg/torture/pr44813.C (revision 0) > *************** > *** 0 **** > --- 1,60 ---- > + typedef unsigned int PRUint32; > + typedef int PRInt32; > + typedef unsigned long PRUint64; > + typedef int PRIntn; > + typedef PRIntn PRBool; > + struct nsRect { > + nsRect(const nsRect& aRect) { } > + }; > + enum nsCompatibility { eCompatibility_NavQuirks = 3 }; > + class gfxContext; > + typedef PRUint64 nsFrameState; > + class nsPresContext { > + public: > + nsCompatibility CompatibilityMode() const { } > + }; > + class nsStyleContext { > + public: > + PRBool HasTextDecorations() const; > + }; > + class nsIFrame { > + public: > + nsPresContext* PresContext() const; > + nsStyleContext* GetStyleContext() const; > + nsFrameState GetStateBits() const; > + nsRect GetOverflowRect() const; > + }; > + class nsFrame : public nsIFrame { }; > + class nsLineList_iterator { }; > + class nsLineList { > + public: > + typedef nsLineList_iterator iterator; > + }; > + class gfxSkipCharsIterator { }; > + class gfxTextRun { > + public: > + class PropertyProvider { }; > + }; > + class nsTextFrame : public nsFrame > + { > + virtual nsRect ComputeTightBounds(gfxContext* aContext) const; > + gfxSkipCharsIterator EnsureTextRun(gfxContext* aReferenceContext = 0L, > + nsIFrame* aLineContainer = 0L, > + const nsLineList::iterator* aLine = 0L, > + PRUint32* aFlowEndInTextRun = 0L); > + }; > + class PropertyProvider : public gfxTextRun::PropertyProvider > + { > + public: > + PropertyProvider(nsTextFrame* aFrame, const gfxSkipCharsIterator& aStart); > + PRInt32 mLength[64]; > + }; > + nsRect nsTextFrame::ComputeTightBounds(gfxContext* aContext) const > + { > + if ((GetStyleContext()->HasTextDecorations() > + && eCompatibility_NavQuirks == PresContext()->CompatibilityMode()) > + || (GetStateBits() & (nsFrameState(1) << (23)))) > + return GetOverflowRect(); > + gfxSkipCharsIterator iter = const_cast<nsTextFrame*>(this)->EnsureTextRun(); > + PropertyProvider provider(const_cast<nsTextFrame*>(this), iter); > + } > Index: tree-ssa-ccp.c > =================================================================== > *** tree-ssa-ccp.c (revision 161826) > --- tree-ssa-ccp.c (working copy) > *************** get_default_value (tree var) > *** 300,306 **** > before being initialized. If VAR is a local variable, we > can assume initially that it is UNDEFINED, otherwise we must > consider it VARYING. */ > ! if (is_gimple_reg (sym) && TREE_CODE (sym) != PARM_DECL) > val.lattice_val = UNDEFINED; > else > val.lattice_val = VARYING; > --- 300,307 ---- > before being initialized. If VAR is a local variable, we > can assume initially that it is UNDEFINED, otherwise we must > consider it VARYING. */ > ! if (is_gimple_reg (sym) > ! && TREE_CODE (sym) == VAR_DECL) > val.lattice_val = UNDEFINED; > else > val.lattice_val = VARYING; > Index: ipa-split.c > =================================================================== > *** ipa-split.c (revision 161826) > --- ipa-split.c (working copy) > *************** split_function (struct split_point *spli > *** 967,997 **** > return_bb == EXIT_BLOCK_PTR ? 0 : EDGE_FALLTHRU); > e->count = call_bb->count; > e->probability = REG_BR_PROB_BASE; > if (return_bb != EXIT_BLOCK_PTR) > { > real_retval = retval = find_retval (return_bb); > if (real_retval > && !is_gimple_min_invariant (retval) > && (TREE_CODE (retval) != SSA_NAME > ! || !SSA_NAME_IS_DEFAULT_DEF (retval))) > { > gimple_stmt_iterator psi; > > ! /* See if there is PHI defining return value. */ > ! for (psi = gsi_start_phis (return_bb); > ! !gsi_end_p (psi); gsi_next (&psi)) > ! if (is_gimple_reg (gimple_phi_result (gsi_stmt (psi)))) > ! break; > ! > ! /* When we have PHI, update PHI. When there is no PHI, > ! update the return statement itself. */ > ! if (TREE_CODE (retval) == SSA_NAME) > { > retval = make_ssa_name (SSA_NAME_VAR (retval), call); > if (TREE_CODE (retval) == SSA_NAME > && !gsi_end_p (psi)) > add_phi_arg (gsi_stmt (psi), retval, e, UNKNOWN_LOCATION); > ! else if (TREE_CODE (retval) == SSA_NAME) > { > gimple_stmt_iterator bsi; > for (bsi = gsi_start_bb (return_bb); !gsi_end_p (bsi); > --- 967,1013 ---- > return_bb == EXIT_BLOCK_PTR ? 0 : EDGE_FALLTHRU); > e->count = call_bb->count; > e->probability = REG_BR_PROB_BASE; > + > + /* If there is return basic block, see what value we need to store > + return value into and put call just before it. */ > if (return_bb != EXIT_BLOCK_PTR) > { > real_retval = retval = find_retval (return_bb); > + > + /* See if return value is computed by split part; > + function might just return its argument, invariant or undefined > + value. In this case we don't need to do any updating. */ > if (real_retval > && !is_gimple_min_invariant (retval) > && (TREE_CODE (retval) != SSA_NAME > ! || (!SSA_NAME_IS_DEFAULT_DEF (retval) > ! || DECL_BY_REFERENCE > ! (DECL_RESULT (current_function_decl))))) > { > gimple_stmt_iterator psi; > > ! /* See if we need new SSA_NAME for the result. > ! When DECL_BY_REFERENCE is true, retval is actually pointer to > ! return value and it is constant in whole function. */ > ! if (TREE_CODE (retval) == SSA_NAME > ! && !DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))) > { > retval = make_ssa_name (SSA_NAME_VAR (retval), call); > + > + /* See if there is PHI defining return value. */ > + for (psi = gsi_start_phis (return_bb); > + !gsi_end_p (psi); gsi_next (&psi)) > + if (is_gimple_reg (gimple_phi_result (gsi_stmt (psi)))) > + break; > + > + /* When there is PHI, just update its value. */ > if (TREE_CODE (retval) == SSA_NAME > && !gsi_end_p (psi)) > add_phi_arg (gsi_stmt (psi), retval, e, UNKNOWN_LOCATION); > ! /* Otherwise update the return BB itself. > ! find_return_bb allows at most one assignment to return value, > ! so update first statement. */ > ! else > { > gimple_stmt_iterator bsi; > for (bsi = gsi_start_bb (return_bb); !gsi_end_p (bsi); > *************** split_function (struct split_point *spli > *** 1016,1021 **** > --- 1032,1040 ---- > } > gsi_insert_after (&gsi, call, GSI_NEW_STMT); > } > + /* We don't use return block (there is either no return in function or > + multiple of them). So create new basic block with return statement. > + */ > else > { > gimple ret; > *************** split_function (struct split_point *spli > *** 1030,1036 **** > && !DECL_BY_REFERENCE (retval)) > retval = create_tmp_reg (TREE_TYPE (retval), NULL); > if (is_gimple_reg (retval)) > ! retval = make_ssa_name (retval, call); > if (DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))) > gimple_call_set_lhs (call, build_simple_mem_ref (retval)); > else > --- 1049,1076 ---- > && !DECL_BY_REFERENCE (retval)) > retval = create_tmp_reg (TREE_TYPE (retval), NULL); > if (is_gimple_reg (retval)) > ! { > ! /* When returning by reference, there is only one SSA name > ! assigned to RESULT_DECL (that is pointer to return value). > ! Look it up or create new one if it is missing. */ > ! if (DECL_BY_REFERENCE (retval)) > ! { > ! tree retval_name; > ! if ((retval_name = gimple_default_def (cfun, retval)) > ! != NULL) > ! retval = retval_name; > ! else > ! { > ! retval_name = make_ssa_name (retval, > ! gimple_build_nop ()); > ! set_default_def (retval, retval_name); > ! retval = retval_name; > ! } > ! } > ! /* Otherwise produce new SSA name for return value. */ > ! else > ! retval = make_ssa_name (retval, call); > ! } > if (DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))) > gimple_call_set_lhs (call, build_simple_mem_ref (retval)); > else > Index: tree-ssa.c > =================================================================== > *** tree-ssa.c (revision 161826) > --- tree-ssa.c (working copy) > *************** verify_def (basic_block bb, basic_block > *** 638,643 **** > --- 638,650 ---- > if (verify_ssa_name (ssa_name, is_virtual)) > goto err; > > + if (TREE_CODE (SSA_NAME_VAR (ssa_name)) == RESULT_DECL > + && DECL_BY_REFERENCE (SSA_NAME_VAR (ssa_name))) > + { > + error ("RESULT_DECL should be read only when DECL_BY_REFERENCE is set."); > + goto err; > + } > + > if (definition_block[SSA_NAME_VERSION (ssa_name)]) > { > error ("SSA_NAME created in two different blocks %i and %i", > Index: tree-inline.c > =================================================================== > *** tree-inline.c (revision 161826) > --- tree-inline.c (working copy) > *************** remap_gimple_stmt (gimple stmt, copy_bod > *** 1236,1242 **** > If RETVAL is just the result decl, the result decl has > already been set (e.g. a recent "foo (&result_decl, ...)"); > just toss the entire GIMPLE_RETURN. */ > ! if (retval && TREE_CODE (retval) != RESULT_DECL) > { > copy = gimple_build_assign (id->retvar, retval); > /* id->retvar is already substituted. Skip it on later remapping. */ > --- 1237,1246 ---- > If RETVAL is just the result decl, the result decl has > already been set (e.g. a recent "foo (&result_decl, ...)"); > just toss the entire GIMPLE_RETURN. */ > ! if (retval > ! && (TREE_CODE (retval) != RESULT_DECL > ! && (TREE_CODE (retval) != SSA_NAME > ! || TREE_CODE (SSA_NAME_VAR (retval)) != RESULT_DECL))) > { > copy = gimple_build_assign (id->retvar, retval); > /* id->retvar is already substituted. Skip it on later remapping. */ > Index: tree-ssa-structalias.c > =================================================================== > *** tree-ssa-structalias.c (revision 161826) > --- tree-ssa-structalias.c (working copy) > *************** get_constraint_for_ssa_var (tree t, VEC( > *** 2836,2842 **** > /* For parameters, get at the points-to set for the actual parm > decl. */ > if (TREE_CODE (t) == SSA_NAME > ! && TREE_CODE (SSA_NAME_VAR (t)) == PARM_DECL > && SSA_NAME_IS_DEFAULT_DEF (t)) > { > get_constraint_for_ssa_var (SSA_NAME_VAR (t), results, address_p); > --- 2836,2844 ---- > /* For parameters, get at the points-to set for the actual parm > decl. */ > if (TREE_CODE (t) == SSA_NAME > ! && (TREE_CODE (SSA_NAME_VAR (t)) == PARM_DECL > ! || (TREE_CODE (SSA_NAME_VAR (t)) == RESULT_DECL > ! && DECL_BY_REFERENCE (SSA_NAME_VAR (t)))) There shouldn't be a default-def for non-by-reference result decls, so no need to check the flag here or below. Ok with that change. Thanks, Richard. > && SSA_NAME_IS_DEFAULT_DEF (t)) > { > get_constraint_for_ssa_var (SSA_NAME_VAR (t), results, address_p); > *************** get_fi_for_callee (gimple call) > *** 3982,3988 **** > if (TREE_CODE (decl) == SSA_NAME) > { > if (TREE_CODE (decl) == SSA_NAME > ! && TREE_CODE (SSA_NAME_VAR (decl)) == PARM_DECL > && SSA_NAME_IS_DEFAULT_DEF (decl)) > decl = SSA_NAME_VAR (decl); > return get_vi_for_tree (decl); > --- 3984,3992 ---- > if (TREE_CODE (decl) == SSA_NAME) > { > if (TREE_CODE (decl) == SSA_NAME > ! && (TREE_CODE (SSA_NAME_VAR (decl)) == PARM_DECL > ! || (TREE_CODE (SSA_NAME_VAR (decl)) == RESULT_DECL > ! && DECL_BY_REFERENCE (SSA_NAME_VAR (decl)))) > && SSA_NAME_IS_DEFAULT_DEF (decl)) > decl = SSA_NAME_VAR (decl); > return get_vi_for_tree (decl); > *************** find_what_p_points_to (tree p) > *** 5751,5757 **** > /* For parameters, get at the points-to set for the actual parm > decl. */ > if (TREE_CODE (p) == SSA_NAME > ! && TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL > && SSA_NAME_IS_DEFAULT_DEF (p)) > lookup_p = SSA_NAME_VAR (p); > > --- 5755,5763 ---- > /* For parameters, get at the points-to set for the actual parm > decl. */ > if (TREE_CODE (p) == SSA_NAME > ! && (TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL > ! || (TREE_CODE (SSA_NAME_VAR (p)) == RESULT_DECL > ! && DECL_BY_REFERENCE (SSA_NAME_VAR (p)))) > && SSA_NAME_IS_DEFAULT_DEF (p)) > lookup_p = SSA_NAME_VAR (p); > > >
On Mon, Jul 5, 2010 at 9:18 AM, Jan Hubicka <hubicka@ucw.cz> wrote: > Hi, > here is updated patch, with Richard's reduced testcase that solves some furhter > problems. > In particular adding the tester down that ipa-split produce new SSA name > for return value and sets as definining statement the call (that is wrong > for DECL_BY_REFERENCE)> I also cleaned up the code a bit there. > Also ipa-ssa-ccp considers RESULT_DECL as undefined that leads to misoptimization > after fixing the first problem on libstdc++ vector testcase. > > Bootstrapped/regtested x86_64-linux, OK? > > * tree-ssa-uninit.c (ssa_undefined_value_p): Result decl is defined > for functions passed by reference. > * tree.c (needs_to_live_in_memory): RESULT_DECL don't need to live > in memory when passed by reference. > * tree-ssa-ccp.c (get_default_value): Only VAR_DECL is undefined at > beggining. > * ipa-split.c (split_function): Cleanup way return value is passed; > handle SSA DECL_BY_REFERENCE retvals. > * tree-ssa.c (verify_def): Verify that RESULT_DECL is read only when > DECL_BY_REFERENCE is set. > * tree-inline.c (remap_gimple_stmt): Handle SSA DECL_BY_REFERENCE > returns. > * tree-ssa-structalias.c (get_constraint_for_ssa_var, get_fi_for_callee, > find_what_p_points_to): Handle RESULT_DECL. > This caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47313
Index: tree-ssa-uninit.c =================================================================== --- tree-ssa-uninit.c (revision 161774) +++ tree-ssa-uninit.c (working copy) @@ -92,6 +92,12 @@ ssa_undefined_value_p (tree t) if (TREE_CODE (var) == PARM_DECL) return false; + /* When returning by reference the return address is actually a hidden + parameter. */ + if (TREE_CODE (SSA_NAME_VAR (t)) == RESULT_DECL + && DECL_BY_REFERENCE (SSA_NAME_VAR (t))) + return false; + /* Hard register variables get their initial value from the ether. */ if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var)) return false; Index: tree.c =================================================================== --- tree.c (revision 161774) +++ tree.c (working copy) @@ -9750,6 +9750,7 @@ 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))); } Index: tree-inline.c =================================================================== --- tree-inline.c (revision 161774) +++ tree-inline.c (working copy) @@ -1236,7 +1237,11 @@ remap_gimple_stmt (gimple stmt, copy_bod If RETVAL is just the result decl, the result decl has already been set (e.g. a recent "foo (&result_decl, ...)"); just toss the entire GIMPLE_RETURN. */ - if (retval && TREE_CODE (retval) != RESULT_DECL) + if (retval + && (TREE_CODE (retval) != RESULT_DECL + && (TREE_CODE (retval) != SSA_NAME + || !SSA_NAME_IS_DEFAULT_DEF (retval) + || TREE_CODE (SSA_NAME_VAR (retval)) != RESULT_DECL))) { copy = gimple_build_assign (id->retvar, retval); /* id->retvar is already substituted. Skip it on later remapping. */ Index: tree-ssa-structalias.c =================================================================== --- tree-ssa-structalias.c (revision 161774) +++ tree-ssa-structalias.c (working copy) @@ -5751,7 +5751,8 @@ find_what_p_points_to (tree p) /* For parameters, get at the points-to set for the actual parm decl. */ if (TREE_CODE (p) == SSA_NAME - && TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL + && (TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL + || TREE_CODE (SSA_NAME_VAR (p)) == RESULT_DECL) && SSA_NAME_IS_DEFAULT_DEF (p)) lookup_p = SSA_NAME_VAR (p); Index: tree-cfg.c =================================================================== --- tree-cfg.c (revision 161774) +++ tree-cfg.c (working copy) @@ -3818,8 +3818,7 @@ verify_gimple_return (gimple stmt) if (op == NULL) return false; - if (!is_gimple_val (op) - && TREE_CODE (op) != RESULT_DECL) + if (!is_gimple_val (op) && TREE_CODE (op) != RESULT_DECL) { error ("invalid operand in return statement"); debug_generic_stmt (op); @@ -3829,7 +3828,10 @@ verify_gimple_return (gimple stmt) if (!useless_type_conversion_p (restype, TREE_TYPE (op)) /* ??? With C++ we can have the situation that the result decl is a reference type while the return type is an aggregate. */ - && !(TREE_CODE (op) == RESULT_DECL + && !((TREE_CODE (op) == RESULT_DECL + || (TREE_CODE (op) == SSA_NAME + && SSA_NAME_IS_DEFAULT_DEF (op) + && TREE_CODE (SSA_NAME_VAR (op)) == RESULT_DECL)) && TREE_CODE (TREE_TYPE (op)) == REFERENCE_TYPE && useless_type_conversion_p (restype, TREE_TYPE (TREE_TYPE (op))))) {