[dataflow]: PATCH COMMITTED to fix frame pointer use in dce.

[Kenneth Zadeck]

Richard Sandiford wrote:
> Kenneth Zadeck <zadeck@naturalbridge.com> writes:
>   
>> This patch fixes the frame pointer handling in dce according to the
>> discussion on irc between ian taylor, richard sandiford, seongae park,
>> danny berlin and myself.
>>     
>
> ENOPATCH btw.  But I can guess what it did from the changelog.
>
>   
>> The resolution was that the frame_pointer would be made an artificial
>> use after reload if FRAME_POINTER_NEEDED was true and that frame related
>> insns could be deleted if they had a REG_MAYBE_DEAD note attached. 
>>     
>
> Sorry to open old wounds, but...
>
>   
>>     * dce.c (deletable_insn_p): Only allow frame-related insns to be
>>     deleted if there is a REG_MAYBE_DEAD note.
>>     
>
> ...I'm surprised that we're still checking RTX_FRAME_RELATED_P at
> all here.  I know that's what you and Ian were talking about early
> on in the conversation, but I'd hoped that the "consider the frame
> pointer to be live if frame_pointer_needed" idea had superceded it.
>
> You originally added the RTX_FRAME_RELATED_P check to stop a frame
> pointer set from being deleted, but now that we're dealing with that
> properly, I thought there were no other known cases where dce would
> wrongly assume that a frame-related insn was dead.
>
> As I said on IRC, I object to RTX_FRAME_RELATED_P being treated
> specially as a correctness measure because (a) it smacks too much
> of magic and (b) there's no guarantee that the (unknown) situations
> that need this magic don't occur for non-RTX_FRAME_RELATED_P insns too.
> (For those who didn't see the IRC debate, my objection was that
> restorations of the frame pointer after a non-local goto might still
> (wrongly) be deleted as dead with the original patch.)
>
> I strongly believe that what flow does is right here.  It proceeds
> as normal, assuming that prologue and epilogue insns don't need to be
> checked for specially, regardless of whether they have a REG_MAYBE_DEAD
> note.  It then aborts if it finds that it is trying to delete such an
> insn.  I think this is a useful sanity check for two reasons: it makes
> sure dce really does understand the semantics of what it's seeing,
> and it detects wrong-code bugs in the backend prologue and epilogue
> generators.  (And I have seen cases where the flow check has caught
> the latter sort of problem.  It really can be very useful.)
>
>   
I have attached the discussion for the record since irc discussions are
not generally kept. 

In the end I will do what the back end maintainers tell me to do.  I
thought that what I had done was consistent with the consensus reached
in that chat and given that one of the participants was a back end
maintainer.   I, of course, may not have interpreted the discussion
properly. 

It sounds like you are saying that none of these frame related insns
would be generated if frame_pointer_needed was false.  If that is true,
the test really is unneeded.   My interpretation of the discussion is
that these insns may be generated irregardless of the setting of
frame_pointer_needed.  However, i have no first hand knowledge of this. 
> Richard
>   
[14:22] <zadeck> rsandifo ping
[14:22] <rsandifo> zadeck: here
[14:23] -->| nms (~nathan@82-46-88-115.cable.ubr10.azte.blueyonder.co.uk
<mailto:%7Enathan@82-46-88-115.cable.ubr10.azte.blueyonder.co.uk>) has
joined #gcc <irc://irc.oftc.net/%23gcc>
[14:24] <zadeck> so i just mini tested the following:
/* These insns may not have real uses but are there because the
dwarf unwinder may need to see the values they compute. */
if (RTX_FRAME_RELATED_P (insn)
&& find_reg_note (insn, REG_MAYBE_DEAD, NULL_RTX) == 0)
return false;
and it does not appear to regress the test that caused us to add the
frame related test.
[14:24] |<-- nms has left irc.oftc.net ()
[14:24] <zadeck> do you think that there is more that needs to be done?
[14:25] <rsandifo> I don't think that's in the spirit of the thing. I
think we're supposed to go ahead and process the insns as normal, and
abort if we end up trying to delete a prologue or epilogue insn that
doesn't have the note.
[14:25] <segher> apinski: the udivdi3 thing is really a kernel bug,
whatever way you look at it. it could be a GCC shortcoming as well (bad
code selection), but the *bug* is the kernel's
[14:26] <rsandifo> I'm not sure what sort of frame-related insn you are
wanting to stop dce from deleting, but it sounds like we need to check
for it semantically, rather than using RTX_FRAME_RELATED_P.
[14:26] <rsandifo> (WAG based on few facts. ;))
[14:27] <iant_work> rsandifo: in this case the insn being deleted is the
one which sets the frame pointer
[14:27] <apinski> segher: I know which is what I exactly mentioned in
the bug report but I guess this guy thinks I am out to get him
[14:27] <iant_work> rsandifo: the frame pointer is not used in the function
[14:27] -->| boegel (~boegel@86-39-24-43.customer.fulladsl.be
<mailto:%7Eboegel@86-39-24-43.customer.fulladsl.be>) has joined #gcc
<irc://irc.oftc.net/%23gcc>
[14:27] <iant_work> rsandifo: but if it is not set then the exception
unwinder fails to restore the saved registers correctly
[14:28] <segher> apinski: yeah, he is one of those guys
[14:28] <zadeck> iant_work, i went back and looked back at that
rountine. It turns out that the insn that i was deleting is not actually
frame related.
[14:28] <apinski> hmm, when did varargs-4.c start to fail on
powerpc64-linux-gnu?
[14:28] <iant_work> zadeck: in the example you sent me, it was; the RTL
was marked with the /f flag
[14:29] <rsandifo> iant_work: Ah. Then how does the EH info refer to the
frame pointer, if the insns themselves don't? Via a FRAME_RELATED_EXPR note?
[14:30] <zadeck> you are correct, but it does not have a
REG_FRAME_RELATED_EXPR note
[14:30] <iant_work> rsandifo: Look at dwarf2out_frame_debug_expr in
dwarf2out.c
[14:30] <zadeck> i am confused.
[14:30] <iant_work> zadeck: No REG_FRAME_RELATED_EXPR note is required
[14:30] <zadeck> what are the notes for then?
[14:30] <iant_work> Those notes are only needed for special cases when
RTX_FRAME_RELATED_P is not enough for dwarf2out.c to figure out what is
happening
[14:31] <iant_work> for example, if you have a type of register which
does not support memory stores, then you need a REG_FRAME_RELATED_EXPR
to explain what is happening when you copy the register to a GPR and
then do the memory store
[14:31] <rsandifo> iant_work: But what specifically about that function?
If we delete the insn that refers to the frame pointer, how does
dwarf2out_frame_debug_expr introduce uses of it?
[14:31] <iant_work> it doesn't
[14:32] <apinski> grrr
[14:32] <iant_work> but the problem then is that the function calls alloca
[14:32] <iant_work> the unwinder tries to use the stack pointer to
restore the registers
[14:32] <iant_work> but that fails because the stack pointer has changed
and the offsets stored by dwarf2out.c point to the wrong place
[14:32] <rsandifo> Ah. Then isn't that what we should be checking for?
That's the kind of thing I meant by a semantic check.
[14:33] <iant_work> what do you want to check for, exactly?
[14:33] <rsandifo> Deleting sets of the frame pointer if flag_exceptions
(or whatever the right condition is)?
[14:34] <apinski> how many testsuite failures can we get on
powerpc64-linux-gnu now?
[14:34] <iant_work> OK, maybe, but isn't that overly tricky?
[14:34] <iant_work> what are you trying to gain?
[14:36] <rsandifo> Well, if we didn't have unwind information to worry
about, deleting the frame pointer set would be a good thing.
[14:37] <iant_work> OK, that is true, but that frame pointer set is only
going to be there for functions which call alloca anyhow
[14:37] <rsandifo> but we set RTX_FRAME_RELATED_P unconditionally AIUI.
[14:37] <iant_work> so what we are talking about at this point is saving
one register-to-register move for a relatively small number of functions
[14:37] <iant_work> and the risk on the other side is that we may not
understand all the cases
[14:38] <iant_work> but if you want to try to pin down all the cases
that is certainly OK with me
[14:38] <iant_work> I'm not confident that I know them all
[14:38] <rsandifo> OK, I get the message ;)
[14:38] <zadeck> my i interject something here
[14:39] <rsandifo> You're the maintainer, so it's your call. I certainly
don't have time to work on it myself. I was just surprised to see such a
blanket check.
[14:39] <zadeck> i added code to only mark the insn as not deletable if
rtx_frame_related_p and there is no reg_maybe_dead note.
[14:40] <DannyB> zadeck: in the future maybe you will learn to just stop
deleting insns if they look unused
[14:40] <DannyB> CAUSE WHO KNOWS
[14:40] <DannyB> i mean, really, who are you to play god with the
instruction stream
[14:40] <zadeck> i tested this with the bug that had motivated you to
have me add the frame_related_p test in the first place and the test passes
[14:40] <iant_work> zadeck: that makes sense to me
[14:40] <DannyB> decide which insns get deleted, and which will live
[14:40] <DannyB> this is not the work of humans
[14:40] <zadeck> this seems to be consistent with the note in reg-notes.def
/* Indicates that this insn (which is part of the prologue) computes a
value which might not be used later, and if so it's OK to delete
the insn. Normally, deleting any insn in the prologue is an error.
At present the parameter is unused and set to (const_int 0). */
[14:41] <iant_work> I don't know why y'all keep thinking the compiler
should represent ALL the dependencies
[14:41] <iant_work> Some dependencies are just obscure
[14:41] <iant_work> And that's the way they like it
[14:41] <zadeck> insecure about your tenure at google?
[14:42] <echristo> hahaha
[14:42] <iant_work> Not Google, just being a gcc developer
[14:42] <iant_work> we can't let just anybody learn this stuff
[14:42] <hpa> Hehehehe
[14:43] <zadeck> seriously: the only thing that bothers me about making
this change is that the insn that i deleted that "seemed" to cause the
faulure" does not have a REG_MAYBE_DEAD note.
[14:43] <iant_work> that's correct: the register is not "maybe dead", so
you should not delete the insn
[14:44] <rsandifo> zadeck: Do you know why flow didn't delete it? Flow
doesn't check for RTX_FRAME_RELATED_P either AFAICT.
[14:44] <zadeck> my dyslexia shows again
[14:44] <iant_work> rsandifo: fair question
[14:44] <iant_work> flow.c just checks frame_pointer_needed
[14:45] <iant_work> but of course we won't be setting the frame pointer
anyhow if !frame_pointer_needed
[14:46] <rsandifo> Right.
[14:47] <zadeck> i think the test that i have if rtx_frame_related_p and
there is no reg_maybe_dead note is the most general, is that what you
people recomend that i test and eventually commit?
[14:47] <iant_work> yes
[14:48] <zadeck> thanks
[14:49] <rsandifo> iant_work: What happens if we restore the frame
pointer from a non-local goto, and that restore is also dead?
[14:49] <iant_work> um, I give up
[14:49] <iant_work> what happens?
[14:49] <rsandifo> Only a problem for C-with-unwind-info, I guess.
[14:50] <iant_work> No, you could have a non-local goto from a nested
function
[14:50] <iant_work> in C++
[14:50] <iant_work> and the enclosing function, which is the target of
the goto, could call alloca
[14:50] <rsandifo> Oh, I didn't think C++ supported nested functions.
[14:50] <iant_work> and then call another function which throws an exception
[14:50] <iant_work> heck if I know what would happen
[14:51] <apinski> C++ does not support nested functins
[14:51] <iant_work> maybe C++ doesn't support nested functions today,
I'm not sure, but I do think we should implement the __lambda extension,
and then it will support them
[14:51] <apinski> but objective-C does and it supports exceptions
[14:51] <DannyB> but objective-C is a piece of crap that breaks if you
look at it wrong
[14:52] <DannyB> so
[14:52] <tromey> __lambda?
[14:52] <rsandifo> So if we just checked for deletions of
frame_pointer_rtx (which I guess it what flow.c does), we cope with the
non-local-goto case too.
[14:52] <tromey> given c++ I can't tell if that is a joke :)
[14:52] <apinski> tromey: a weird C++0x stuff
[14:52] <rsandifo> ...sets to frame_pointer_rtx even
[14:52] <iant_work> rsandifo: OK, good point
[14:52] <DannyB> lambda is not weird
[14:53] <iant_work> zadeck: it may be unwise to delete a set of the
frame pointer if frame_pointer_needed is set
[14:53] <iant_work> even if it appears to be unused
[14:53] <iant_work> tromey: the __lambda idea is pretty much what you
think it is
[14:53] <iant_work> it's not in C++0x and probably won't be but it's
still basically a good idea
[14:53] <iant_work> because STL functors are impossible to comprehend
[14:54] <zadeck> i can easily add that, does that cover the other test
or do we need both?
[14:54] <iant_work> I believe we need both
[14:54] <zadeck> i will do this.
[14:54] <iant_work> although I'm not completely sure
[14:55] <iant_work> I think that if we don't have both the exception
unwinder will sometimes restore the wrong value
[14:55] <iant_work> which would be ungood
[14:55] <iant_work> but I'm not completely sure
[14:55] <rsandifo> I really don't like that though. I think the same
thing applies to other frame-related stuff: i.e. non-frame-related stuff
could do the same thing, and sitll be deleted as dead. So I think we
really do need to pin down what is special about what the insn itself is
doing.
[14:55] <iant_work> with respect to unwinding, the frame pointer is special
[14:56] =-= gccbot has changed the topic to “GCC Development || GOOD
news, GCC 4.2.0 is released || Still 159 regressions in 4.1, 118 in 4.2
and 181 in 4.3 (ssb)”
[14:56] <rsandifo> iant_work: Yeah, but we've already agreed not to
delete sets of the frmae pointer, so I don't understand what you mean.
[14:56] <iant_work> I'm speculating that for a non-local goto there
won't be any non-frame related stuff that we care about
[14:57] <rsandifo> Not even the insn that restores the frame pointer at
the target of the non-local goto?
[14:57] <iant_work> but we agreed not to delete sets of the frame pointer
[14:57] <iant_work> other than that, I mean
[14:57] <iant_work> actually this may have gotten too abstract for me
[14:58] <iant_work> we're basically speculating about those unstated
dependencies
[14:58] <iant_work> in some sense the real answer is to state them
explicitly
[14:58] <iant_work> the only way they are stated now is RTX_FRAME_RELATED_P
[14:58] <iant_work> and frame_pointer_needed
[14:59] <tromey> yeah, I agree about the usefulness of lambda
[14:59] <rsandifo> I take your point that not deleting
RTX_FRAME_RELATED_P is more conservative than not doing it, but I'm
still not convinced that we need it. As evidence: flow hasn't needed it.
And Kenny's testcase wouldn't need it either with our frame_pointer_rtx
check.
[15:00] <iant_work> Actually flow.c has a much stronger check than
RTX_FRAME_RELATED_P
[15:00] <iant_work> flow.c won't delete any insn for which
prologue_epilogue_contains returns true
[15:00] <iant_work> that includes all RTX_FRAME_RELATED_P insns
[15:01] <iant_work> of course, what happens is that if flow thinks it
needs to delete such an insn, it ICEs
[15:01] <rsandifo> Exactly. So I don't get your point. ;)
[15:01] <iant_work> but DCE is smarter than flow and can validly delete
some of those insns
[15:01] <iant_work> so in a sense we are trying to figure out what test
to use instead of prologue_epilogue_contains
[15:01] <rsandifo> But I think we'd understand the situation more if we
left the check out, and added it back later if we found we still need
it. (And could then document _why_ we need it.)
[15:02] <iant_work> which check are you talking about now?
[15:02] <iant_work> the RTX_FRAME_RELATED_P check?
[15:02] <rsandifo> RTX_FRAME_RELATED_P still
[15:02] <iant_work> So you are suggesting that we permit deleting an
RTX_FRAME_RELATED_P insn, but reject deleting a set of the frame pointer
is frame_pointer_needed is true?
[15:02] <rsandifo> Yes, except that I don't understand why
frame_pointer_needed check is needed.
[15:03] <iant_work> we went through that
[15:03] <iant_work> if the frame pointer is not set, and the function
calls alloca, exception unwinding won't work
[15:04] <spark> In general, I don't like frame_pointer_needed type
checking. We should try to model the dataflow - e.g. by modeling alloca
calls using frame pointer.
[15:04] <rsandifo> No, I mean, when would see a set of frame_pointer_rtx
when frame_pointer_needed isn't true?
[15:04] <rsandifo> I'm suggesting not deleting sets of frame_pointer_rtx
full stop.
[15:05] <iant_work> if !frame_pointer_needed, the frame pointer can just
be a GPR
[15:05] <iant_work> in which case deleting sets is OK
[15:05] <rsandifo> But that GPR will be a distinct rtx, not
frame_pointer_rtx.
[15:05] <rsandifo> I.e. they won't be ==
[15:06] <iant_work> I'm skeptical that that is true after reload and
register elimination and prologue and epilogue threading
[15:06] <iant_work> which is the case we are talking about (presumably)
[15:06] <iant_work> that is, I'm skeptical that all uses of the frame
pointer after register elimination actually use frame_pointer_rtx
[15:06] <iant_work> in particular they will use
HARD_FRAME_POINTER_REGNUM rather than FRAME_POINTER_REGNUM
[15:07] -->| boegel (~boegel@86-39-24-43.customer.fulladsl.be
<mailto:%7Eboegel@86-39-24-43.customer.fulladsl.be>) has joined #gcc
<irc://irc.oftc.net/%23gcc>
[15:07] <rsandifo> OK, hard_frame_pointer_rtx, sorry.
[15:07] <rsandifo> But that is explicitly guaranteed by the elimination
initialisation code.
[15:08] <zadeck> dannyb ping
[15:08] <iant_work> rsandifo: you are probably right
[15:08] <DannyB> yes?
[15:08] <rsandifo> /* Count the number of eliminable registers and build
the FROM and TO
[15:08] <rsandifo> REG rtx's. Note that code in gen_rtx_REG will cause,
e.g.,
[15:08] <rsandifo> gen_rtx_REG (Pmode, STACK_POINTER_REGNUM) to equal
stack_pointer_rtx.
[15:08] <rsandifo> We depend on this. */
[15:08] <zadeck> just for the record, if frame_pointer_needed, why don't
we add the frame pointer to the artificial uses?
[15:08] <DannyB> i dunno
[15:09] <iant_work> i give up, why don't you?
[15:09] <rsandifo> Heh
[15:09] <zadeck> we add the frame pointer as an artificial use before reload
[15:09] <spark> zadeck: something along those lines :)
[15:10] <zadeck> i would prefer to add it there rather than just keep
testing it everywhere?
[15:10] <spark> Exactly.
[15:10] <zadeck> s/?/!/
[15:10] |<-- blindvt`_ has left irc.oftc.net (Quit: .)
[15:10] |<-- boegel has left irc.oftc.net ()
[15:11] <zadeck> i think i will try that instead of adding it directly
into dce.
[15:11] <rsandifo> zadeck: Thanks. Sorry for kicking up such a fuss. ;)
[15:11] |<-- jserv-- has left irc.oftc.net (Ping timeout: 480 seconds)
[15:11] <iant_work> rsandifo: thanks for pushing on it
[15:11] <zadeck> this is the way things get fixed
[15:12] <spark> rsandifo: Thanks YOU for bringing this up, cause I
wasn't very happy about it either.