Re: Potential fix for rdar://4658012

[Josh Conner <jconner at apple dot com>]

Richard Kenner wrote:

> I disagree.  Testing is not, and should never be, a substitute for analysis.
> 
> A patch is proposed because we have a reason to believe it's correct.  Then
> we test to increase our confidence that it is, indeed, correct.  But both
> parts are essential for any patch.
> 
> Here we have a situation where we'd like to see some optimization done (the
> merging of temporaries).  There's some code that suppresses that optimization
> in one case because that optimization was unsafe in that case.  It is not
> acceptable to find out whether that code is still needed by merely running
> a test suite: there has to be some analysis that says what changed to
> make that code no longer needed.  The burden of proof on that analysis has
> to be on the person who's proposing that the code is no longer needed.

Richard -

I agree with you -- my intent for originally posting was to hopefully
find that understanding of why the code had originally been added.
Perhaps I should have better explained my own understanding, so that you
could help identify if this was a conceptual mistake on my part.  I
think I had too much confidence that this would be an easy question to
answer, but it appears that the code is quite dated and not well
understood.  So, without further ado, let me provide my analysis, and
hopefully those who understand this code best can help point out if I've
gone astray.  Here is my interpretation of this code:

In the code for managing middle-end-generated temps, it appears that
push/pop_temp_slots is used to create nested contexts into which unique
temps are generated.  When I need a temp, I call push_temp_slots,
allocate a temp, perform my calculation, and call pop_temp_slots when
I'm done.

The one exception to this is if the address of the temp is taken before
I call pop_temp_slots.  In that instance, even though I may be "done"
with the temp, it needs to live until the end of the high-level-language
scope, and so it is marked with addr_taken and is pushed up in the temp
scopes using preserve_temp_slots.  It is this logic that is used for
function calls that return a value on the stack.

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.  So, for these reasons, marking the temp for a
lifetime longer than what is needed by the middle-end for copying it
into the destination, appeared to be excessive.

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.

Thanks again for all of the time looking at this - it's very much
appreciated.

- Josh