This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Potential fix for rdar://4658012


Sorry for my own slow response -- I've been doing more digging through
code, and didn't want to respond without a reasonable understanding...

Richard Kenner wrote:

>> However, in the case where we're passing the address of a temp slot to a
>> function, it doesn't make sense to me that this is the same as other
>> "address-of" operations on a stack slot.  The function call (at least in
>> C and C++) cannot preserve this address, and it is reasonable to say
>> that attempts to access this address after the caller is done with the
>> location, are invalid.
> 
> Here's where I disagree.  The issue isn't what the front-ends (and especially
> not a *subset* of the front-ends) do, but what they *are allowed* to do.
> 
> Going back to 3.4 for a moment, the question is whether you could
> create a valid tree where the address would survive.  And I think you can.
> All it would take is a machine like Sparc that passes all aggregates by
> reference is to have a CALL_EXPR with two operands that are each CALL_EXPRs
> of aggregate types.

Yes, this makes perfect sense.  I can envision a tree where removing
this code would create an invalid reuse of the stack slot.  However, we
are doing two things to preserve this temp -- marking it with
"addr_taken" and creating it with the "keep" bit set.  The former says
"promote this temp one level up", and the latter "keep this temp around
for the lifetime of the current block".  This still seems redundant.  I
think marking the temp as addr_taken is sufficient to handle the case
where the address is used as the input to a function call (see attached
patch).

Note that since Jason's recent change to the frontend here:

  http://gcc.gnu.org/ml/gcc-patches/2006-09/msg00202.html

This change would be sufficient to address the remaining issues in
pr25505.  I'd like to hear your opinion of this, though, before
submitting it for approval.

> We never had a precise specification of what trees were valid then (which
> is precisely the set of valid GENERIC, and hence is similarly not very
> precisely defined), but I think almost everybody working on the 3.4 compiler
> would say that the above is valid whether or not C or C++ could generate it.
> Therefore, the middle-end had to properly support it.
> 
> However, in GCC 4, with tree-ssa, the RTL expanders receive a much smaller
> subset of trees, namely those defined in GIMPLE.  I *believe*, but aren't
> sure, that the above is not valid GIMPLE.
> 
> That would make the change safe.  But:
> 
> (1) The code we generate is pretty inefficient in the current case, since we
> copy the entire aggregate.  If we try to fix that, we *will* be preserving
> the address for later, though perhaps in a temporary more properly allocated.
> 
> (2) I suspect that we can rip out much more of this code than just this line
> and I'd prefer to do it that way.
> 
>> Hopefully this is enough of an explanation to reveal whether my
>> understanding is consistent or inconsistent with the experts, and can
>> move the discussion forward.
> 
> As I said once before, this code didn't change between 3.4 and 4.x, so
> something else is the cause of the regression.  I'd like to understand
> what that was.  I think you posted the changes you propose for the C
> and C++ front ends, but I don't remember what they were.

To help address your concerns about fixing the regression closest to its
point of change, I've also been looking into how this was handled in
3.4.  In that version of the compiler, a TARGET_EXPR was surviving all
the way until the expander, where the function expand_expr_real was
recognizing TARGET_EXPRs and generating a reusable temp.  Unfortunately,
I don't see an obvious correlation in 4.x, as the TARGET_EXPR never
reaches the expander, since gimplification has already converted it into
a CALL_EXPR.

Thanks again for taking the time to look into this.

- Josh


Index: gcc/calls.c
===================================================================
--- gcc/calls.c	(revision 116642)
+++ gcc/calls.c	(working copy)
@@ -1985,7 +1985,7 @@ expand_call (tree exp, rtx target, int i
 	    /* For variable-sized objects, we must be called with a target
 	       specified.  If we were to allocate space on the stack here,
 	       we would have no way of knowing when to free it.  */
-	    rtx d = assign_temp (TREE_TYPE (exp), 1, 1, 1);
+	    rtx d = assign_temp (TREE_TYPE (exp), 0, 1, 1);
 
 	    mark_temp_addr_taken (d);
 	    structure_value_addr = XEXP (d, 0);

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]