| Message ID | BLU0-SMTP43F10F32286A327F4C627297330@phx.gbl |
|---|---|
| State | New |
| Headers | show |
On 05/18/14 09:33, John David Anglin wrote: > The attached change appears to fix PR middle-end/61141. On PA, we can get > deleted insn notes in call sequences. The attached change checks to > make sure we have > a valid insn before calling reset_insn_used_flags and verify_insn_sharing. > > Tested on hppa-unknown-linux-gnu, hppa2.0w-hp-hpux11.11 and > hppa64-hp-hpux11.11. > > OK for trunk? Maybe :-) c#1 you incluide a blob of RTL, but it's not clear where insn 238 was to begin with. ie, was it inside a SEQUENCE or somewhere else? In c#2 you indicate we can get this stuff when an insn in a delay slot sequence is deleted. Where/when are those insns being deleted? Presumably the backends know enough to handle deleted insns in delay slot sequences?!? It's been a long time since I looked at that code in detail, so if you already know it's safe, just say "it's safe" and I'll believe you :-) Otherwise a bit of digging may be needed. My preference would be to remove these insns from the RTL chain, but if that's not going to be reasonably feasible, then we'll probably need to go with something like your patch. Jeff
Hi Jeff, On 19-May-14, at 1:51 PM, Jeff Law wrote: > On 05/18/14 09:33, John David Anglin wrote: >> The attached change appears to fix PR middle-end/61141. On PA, we >> can get >> deleted insn notes in call sequences. The attached change checks to >> make sure we have >> a valid insn before calling reset_insn_used_flags and >> verify_insn_sharing. >> >> Tested on hppa-unknown-linux-gnu, hppa2.0w-hp-hpux11.11 and >> hppa64-hp-hpux11.11. >> >> OK for trunk? > Maybe :-) > > c#1 you incluide a blob of RTL, but it's not clear where insn 238 > was to begin with. ie, was it inside a SEQUENCE or somewhere else? Yes, it was inside a SEQUENCE. The problem compiling c-common.c is very hard to debug. These routines are called intensively and the ICE occurs after several million calls. It takes a couple of hours of running under gdb to reach the failing call to reset_insn_used_flags just counting calls up to ICE. The problem is more apparent with 64-bit compiler due to way PIC register is saved and restored. > > In c#2 you indicate we can get this stuff when an insn in a delay > slot sequence is deleted. Where/when are those insns being deleted? > Presumably the backends know enough to handle deleted insns in delay > slot sequences?!? It's been a long time since I looked at that code > in detail, so if you already know it's safe, just say "it's safe" > and I'll believe you :-) Otherwise a bit of digging may be needed. I believe that the backend must handle the deleted insns in the delay slot as there are are no compilation errors or regressions. However, like you, I'm not 100% certain this done correctly. > > My preference would be to remove these insns from the RTL chain, but > if that's not going to be reasonably feasible, then we'll probably > need to go with something like your patch. Something has changed. Either these insns aren't being removed from RTL chain as they were before or they are being deleted at a much later stage. The same code is present in 4.9 and the problem doesn't seem to occur there. Possibly, a regression search would pin point this. Still, we shouldn't call reset_insn_used_flags or verify_insn_sharing with a note. Dave -- John David Anglin dave.anglin@bell.net
On 05/19/14 15:20, John David Anglin wrote: > The problem compiling c-common.c is very hard to debug. These routines > are called intensively and the ICE occurs after several million calls. > It takes a couple of hours of running under gdb to reach the failing > call to reset_insn_used_flags just counting calls up to ICE. I'd suggest getting a suitable .i/.ii file and debugging it with a cross-debugger ;-) Then again, if you've got a breakpoint that is triggering a million times, it may still take hours to hit when debugging with a cross. > > The problem is more apparent with 64-bit compiler due to way PIC > register is saved and restored. Yea, that makes sense. > >> >> In c#2 you indicate we can get this stuff when an insn in a delay slot >> sequence is deleted. Where/when are those insns being deleted? >> Presumably the backends know enough to handle deleted insns in delay >> slot sequences?!? It's been a long time since I looked at that code >> in detail, so if you already know it's safe, just say "it's safe" and >> I'll believe you :-) Otherwise a bit of digging may be needed. > > I believe that the backend must handle the deleted insns in the delay > slot as there are are no compilation errors or regressions. > However, like you, I'm not 100% certain this done correctly. I'm pretty sure we're getting this wrong in the backend. In fact, the more I think about it, a NOTE_INSN_DELETED in a delay slot is just asking for all kinds of trouble. In general, if we have an insn with a filled delay slot, then we will emit the two insns independently of each other (most of the time, there are exceptions). A NOTE_INSN_DELETED in a delay slot still looks like a filled slot. So the target code isn't going to emit a NOP or anything like that. It's going to leave it up to the generic code to emit the insn with the delay slot and the delay slot insn itself (the NOTE_INSN_DELETED in this case). Of course we don't output anything for a NOTE_INSN_DELETED. So the net result is we, in essence, fill the delay slot with whatever random instruction happens to fall next in the insn chain. Amazingly, nothing seems to be failing, but I've seen far worse bugs go unnoticed for a long time. Sadly,I think we need to start digging deeper to find out what's deleting those insns and take corrective action. Perhaps writing a little routine to peek at all the filled delay slots and squawk if it finds a NOTE_INSN_DELETED. Then call it after each RTL pass that follows reorg in the pass manager. That'd at least narrow it down to a pass that's mucking things up. > Something has changed. Either these insns aren't being removed from RTL > chain as they were before or they are being deleted > at a much later stage. The same code is present in 4.9 and the problem > doesn't seem to occur there. Possibly, a regression search > would pin point this. Probably just gone latent. > > Still, we shouldn't call reset_insn_used_flags or verify_insn_sharing > with a note. Agreed, but at least in this instance, it's the symptom of a deeper problem I think. jeff
On 20-May-14, at 5:23 PM, Jeff Law wrote: >> I believe that the backend must handle the deleted insns in the delay >> slot as there are are no compilation errors or regressions. >> However, like you, I'm not 100% certain this done correctly. > I'm pretty sure we're getting this wrong in the backend. In fact, > the more I think about it, a NOTE_INSN_DELETED in a delay slot is > just asking for all kinds of trouble. > > In general, if we have an insn with a filled delay slot, then we > will emit the two insns independently of each other (most of the > time, there are exceptions). > > A NOTE_INSN_DELETED in a delay slot still looks like a filled slot. > So the target code isn't going to emit a NOP or anything like that. > It's going to leave it up to the generic code to emit the insn with > the delay slot and the delay slot insn itself (the NOTE_INSN_DELETED > in this case). > > Of course we don't output anything for a NOTE_INSN_DELETED. So the > net result is we, in essence, fill the delay slot with whatever > random instruction happens to fall next in the insn chain. > > Amazingly, nothing seems to be failing, but I've seen far worse bugs > go unnoticed for a long time. > > Sadly,I think we need to start digging deeper to find out what's > deleting those insns and take corrective action. > > Perhaps writing a little routine to peek at all the filled delay > slots and squawk if it finds a NOTE_INSN_DELETED. Then call it > after each RTL pass that follows reorg in the pass manager. That'd > at least narrow it down to a pass that's mucking things up. The insns are being deleted in "final" after the assembly output has being done. So, for example, pa_output_call is never called with a delayed branch sequence containing a NOTE_INSN_DELETED. It would seem to me that this "cleanup" should be done before the "dbr" pass. If dbr_sequence() ignored notes, we would emit the nop. However, it doesn't look like it does. It fairly easy to check for this case but I hesitate to add code to handle cases that aren't supposed to happen. Dave -- John David Anglin dave.anglin@bell.net
On 05/24/14 12:39, John David Anglin wrote: > > The insns are being deleted in "final" after the assembly output has > being done. So, for example, pa_output_call is > never called with a delayed branch sequence containing a NOTE_INSN_DELETED. OK. That's good to know. I scanned final and final_scan_insn and see a couple places where we delete insns, but if I'm reading the code correctly, they would result in seeing the NOTE_INSN_DELETED rather than a real insn in the output routines. So what code during final is deleting these insns? Jeff
On 4-Jun-14, at 3:38 AM, Jeff Law wrote: > On 05/24/14 12:39, John David Anglin wrote: >> >> The insns are being deleted in "final" after the assembly output has >> being done. So, for example, pa_output_call is >> never called with a delayed branch sequence containing a >> NOTE_INSN_DELETED. > OK. That's good to know. I scanned final and final_scan_insn and > see a couple places where we delete insns, but if I'm reading the > code correctly, they would result in seeing the NOTE_INSN_DELETED > rather than a real insn in the output routines. > > So what code during final is deleting these insns? It's the PA backend. For example, /* If this isn't a sibcall, we put the load of %r27 into the delay slot. We can't do this in a sibcall as we don't have a second call-clobbered scratch register available. */ if (seq_length != 0 && ! JUMP_P (NEXT_INSN (insn)) && !sibcall) { final_scan_insn (NEXT_INSN (insn), asm_out_file, optimize, 0, NULL); /* Now delete the delay insn. */ SET_INSN_DELETED (NEXT_INSN (insn)); delay_insn_deleted = 1; } The insn is deleted so that it won't be emitted again. This is done in a bunch of places. What's new is the verification after assembly output. Dave -- John David Anglin dave.anglin@bell.net
On 06/08/14 12:06, John David Anglin wrote: > On 4-Jun-14, at 3:38 AM, Jeff Law wrote: > >> On 05/24/14 12:39, John David Anglin wrote: >>> >>> The insns are being deleted in "final" after the assembly output has >>> being done. So, for example, pa_output_call is >>> never called with a delayed branch sequence containing a >>> NOTE_INSN_DELETED. >> OK. That's good to know. I scanned final and final_scan_insn and see >> a couple places where we delete insns, but if I'm reading the code >> correctly, they would result in seeing the NOTE_INSN_DELETED rather >> than a real insn in the output routines. >> >> So what code during final is deleting these insns? > > It's the PA backend. For example, > > /* If this isn't a sibcall, we put the load of %r27 into the > delay slot. We can't do this in a sibcall as we don't > have a second call-clobbered scratch register available. */ > if (seq_length != 0 > && ! JUMP_P (NEXT_INSN (insn)) > && !sibcall) > { > final_scan_insn (NEXT_INSN (insn), asm_out_file, > optimize, 0, NULL); > > /* Now delete the delay insn. */ > SET_INSN_DELETED (NEXT_INSN (insn)); > delay_insn_deleted = 1; > } > > The insn is deleted so that it won't be emitted again. This is done in > a bunch > of places. > > What's new is the verification after assembly output. Ah. OK. Glad to see I wasn't missing something in final.c. Let me review things again, but I'm a *lot* less worried now :-) Thanks for digging into this! jeff
On 06/08/14 12:06, John David Anglin wrote: > On 4-Jun-14, at 3:38 AM, Jeff Law wrote: > >> On 05/24/14 12:39, John David Anglin wrote: >>> >>> The insns are being deleted in "final" after the assembly output has >>> being done. So, for example, pa_output_call is >>> never called with a delayed branch sequence containing a >>> NOTE_INSN_DELETED. >> OK. That's good to know. I scanned final and final_scan_insn and see >> a couple places where we delete insns, but if I'm reading the code >> correctly, they would result in seeing the NOTE_INSN_DELETED rather >> than a real insn in the output routines. >> >> So what code during final is deleting these insns? > > It's the PA backend. For example, > > /* If this isn't a sibcall, we put the load of %r27 into the > delay slot. We can't do this in a sibcall as we don't > have a second call-clobbered scratch register available. */ > if (seq_length != 0 > && ! JUMP_P (NEXT_INSN (insn)) > && !sibcall) > { > final_scan_insn (NEXT_INSN (insn), asm_out_file, > optimize, 0, NULL); > > /* Now delete the delay insn. */ > SET_INSN_DELETED (NEXT_INSN (insn)); > delay_insn_deleted = 1; > } > > The insn is deleted so that it won't be emitted again. This is done in > a bunch > of places. > > What's new is the verification after assembly output. Right. Thanks for the analysis. I know it takes significant time. The original patch is fine and I've installed it on your behalf. Thanks again, Jeff
On 6/9/2014 12:41 PM, Jeff Law wrote: >> What's new is the verification after assembly output. > Right. Thanks for the analysis. I know it takes significant time. Not a problem. Thanks for installing the change. Dave
Index: emit-rtl.c =================================================================== --- emit-rtl.c (revision 210323) +++ emit-rtl.c (working copy) @@ -2698,7 +2698,11 @@ { gcc_assert (REG_NOTES (p) == NULL); for (int i = 0; i < XVECLEN (pat, 0); i++) - reset_insn_used_flags (XVECEXP (pat, 0, i)); + { + rtx insn = XVECEXP (pat, 0, i); + if (INSN_P (insn)) + reset_insn_used_flags (insn); + } } } } @@ -2735,7 +2739,11 @@ verify_insn_sharing (p); else for (int i = 0; i < XVECLEN (pat, 0); i++) - verify_insn_sharing (XVECEXP (pat, 0, i)); + { + rtx insn = XVECEXP (pat, 0, i); + if (INSN_P (insn)) + verify_insn_sharing (insn); + } } reset_all_used_flags ();