| Message ID | 4CA11A25.5090901@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 2010-09-27 at 15:26 -0700, Richard Henderson wrote: > On 09/27/2010 12:27 PM, Richard Henderson wrote: > > On 09/27/2010 09:09 AM, Steve Ellcey wrote: > >> I tried using this patch and the stage2/stage3 comparision failed on > >> most files, for both ia64 HP-UX and Linux. If I compile cfg.o to a .s > >> file and do a diff I see: > > > > Got it. The real difference is: > > > > alloc r34 = ar.pfs, 1, 3, 1, 0 > > +.LCFI0: > > addl r14 = @ltoffx(_xexit_cleanup#), r1 > > > > i.e. the dwarf2 cfi stuff isn't creating debug labels, > > which means we break the bundle and add nops where we > > didn't want em. > > > > Hopefully I'll have a new patch this afternoon so you > > can do overnight tests. > > Whee. Tested along with the T_U_I patch on ia64-linux > and committed. > > > r~ I got good bootstraps on IA64 HP-UX and Linux but I don't know if there are any regressions because I am getting lots of failures with /proj/opensrc/nightly/src/trunk/gcc/testsuite/gcc.c-torture/compile/20000105-1.c:25:1: sorry, unimplemented: gimple bytecode streams do not support machine specific builtin functions on this target This is unrelated to your patch but is due to Richard Guenther's patch to always use TRANSLATION_UNIT_DECL as context. Steve Ellcey sje@cup.hp.com
Hello, On 28.09.2010 22:42, Steve Ellcey wrote: > On Mon, 2010-09-27 at 15:26 -0700, Richard Henderson wrote: >> On 09/27/2010 12:27 PM, Richard Henderson wrote: >>> On 09/27/2010 09:09 AM, Steve Ellcey wrote: >>>> I tried using this patch and the stage2/stage3 comparision failed on >>>> most files, for both ia64 HP-UX and Linux. If I compile cfg.o to a .s >>>> file and do a diff I see: >>> >>> Got it. The real difference is: >>> >>> alloc r34 = ar.pfs, 1, 3, 1, 0 >>> +.LCFI0: >>> addl r14 = @ltoffx(_xexit_cleanup#), r1 >>> >>> i.e. the dwarf2 cfi stuff isn't creating debug labels, >>> which means we break the bundle and add nops where we >>> didn't want em. >>> >>> Hopefully I'll have a new patch this afternoon so you >>> can do overnight tests. >> >> Whee. Tested along with the T_U_I patch on ia64-linux >> and committed. >> >> >> r~ > > I got good bootstraps on IA64 HP-UX and Linux but I don't know if there > are any regressions because I am getting lots of failures with This patch fails with selective scheduling on the attached testcase, distilled from gengtype.c. If you do ./gcc/cc1 -quiet -g -O2 -fselective-scheduling2 out.i you get out.i: In function ‘output_type_enum’: out.i:64:1: internal compiler error: in dwarf2out_cfi_begin_epilogue, at dwarf2out.c:2930 The scheduler lifts an insn (ar.pfs = r35) from the epilogue creating a bookkeeping copy and thus creating an extra NOTE_INSN_EPILOGUE_BEG note. If this is not permitted now, what do I check to prevent that happening? Andrey typedef long unsigned int size_t; struct fileloc { const char *file; }; typedef struct type *type_p; typedef const struct type *const_type_p; enum typekind { TYPE_STRUCT, TYPE_UNION, TYPE_POINTER, TYPE_LANG_STRUCT, TYPE_PARAM_STRUCT }; struct type { enum typekind kind; union { struct { struct fileloc line; } s; struct { struct fileloc line; } param_struct; } u; }; struct outf { size_t bufused; char *buf; }; typedef struct outf *outf_p; oprintf (outf_p o, const char *format, ...) { char *s; size_t slength; memcpy (o->buf + o->bufused, s, slength); } output_mangled_typename (outf_p of, const_type_p t) { switch (t->kind) { case TYPE_POINTER: (fancy_abort ("/home/bonzo/develop/trunk-git/gcc/gengtype.c", 1988, __FUNCTION__)); } } output_type_enum (outf_p of, type_p s) { if (s->kind == TYPE_PARAM_STRUCT && s->u.param_struct.line.file != ((void *)0)) { oprintf (of, ", gt_e_"); } else if (((s)->kind == TYPE_UNION || (s)->kind == TYPE_STRUCT || (s)->kind == TYPE_LANG_STRUCT) && s->u.s.line.file != ((void *)0)) { oprintf (of, ", gt_ggc_e_"); output_mangled_typename (of, s); } else oprintf (of, ", gt_types_enum_last"); }
On 09/30/2010 05:46 AM, Andrey Belevantsev wrote: > The scheduler lifts an insn (ar.pfs = r35) from the epilogue creating > a bookkeeping copy and thus creating an extra NOTE_INSN_EPILOGUE_BEG > note. If this is not permitted now, what do I check to prevent that > happening? The NOTE_INSN_EPILOGUE_BEG needs to be strictly paired with the return/sibcall insn that exits the function. In practice I think this means it cannot be moved out of the (copy of) the block into which it was emitted. It will also be an error to move an insn marked RTX_FRAME_RELATED_P before the EPILOGUE_BEG note. The rest of the insns emitted with the epilogue don't really matter. In the case of the ar.pfs move -- which isn't FRAME_RELATED -- I would say that the best thing to do is simply remove it from the epilogue_insn_hash and not move the EPILOGUE_BEG note. This will require a new interface in function.c. You got away with this before on ia64 because the ia64 unwind info ties emission of the copy_state/label_state not to the EPILOGUE_BEG note, but rather to the first (and only) FRAME_RELATED_P epilogue insn. Which is, of course, the stack pointer restore. And if we have one of those we will have generated a BLOCKAGE which cut off your scheduling freedom anyway. r~
On 30.09.2010 20:24, Richard Henderson wrote: > On 09/30/2010 05:46 AM, Andrey Belevantsev wrote: >> The scheduler lifts an insn (ar.pfs = r35) from the epilogue creating >> a bookkeeping copy and thus creating an extra NOTE_INSN_EPILOGUE_BEG >> note. If this is not permitted now, what do I check to prevent that >> happening? > > The NOTE_INSN_EPILOGUE_BEG needs to be strictly paired with the > return/sibcall insn that exits the function. In practice I think > this means it cannot be moved out of the (copy of) the block into > which it was emitted. > > It will also be an error to move an insn marked RTX_FRAME_RELATED_P > before the EPILOGUE_BEG note. The rest of the insns emitted with > the epilogue don't really matter. > > In the case of the ar.pfs move -- which isn't FRAME_RELATED -- I > would say that the best thing to do is simply remove it from the > epilogue_insn_hash and not move the EPILOGUE_BEG note. This will > require a new interface in function.c. I was wrong, the problem was not in creating an extra copy of NOTE_INSN_EPILOGUE_BEG, but rather in moving one note up such that there was a control flow path with two such notes (the function had two returns from the beginning). Anyway, currently any prologue_epilogue_contains insn can be moved out of its block if this would not require a bookkeeping copy. This is not enough to prevent the above cases to happen, thus I can stop moving both RTX_FRAME_RELATED_P insns and NOTE_INSN_EPILOGUE_BEG notes from their basic block. For the latter, I need to stop moving the insn to which the note was attached in remove_notes, and this happen to be the non-frame related ar.pfs move; removing this move from epilogue_insn_hash would not help as this would only increase the scheduling freedom that we have. The patch implementing the above in sel-sched works for the test case. Does this sound right to you? Andrey
On 10/01/2010 02:46 AM, Andrey Belevantsev wrote: > For the latter, I need to stop moving > the insn to which the note was attached in remove_notes, and this > happen to be the non-frame related ar.pfs move; removing this move > from epilogue_insn_hash would not help as this would only increase > the scheduling freedom that we have. I thought we removed the prologue and epilogue notes during scheduling and replaced them with reposition_p_a_e_notes. But what you're saying is that the note is still attached to its adjacent insn and repositioned later? > The patch implementing the above in sel-sched works for the test > case. Does this sound right to you? It sounds plausible. r~
On 01.10.2010 19:06, Richard Henderson wrote: > On 10/01/2010 02:46 AM, Andrey Belevantsev wrote: >> For the latter, I need to stop moving >> the insn to which the note was attached in remove_notes, and this >> happen to be the non-frame related ar.pfs move; removing this move >> from epilogue_insn_hash would not help as this would only increase >> the scheduling freedom that we have. > > I thought we removed the prologue and epilogue notes during > scheduling and replaced them with reposition_p_a_e_notes. > But what you're saying is that the note is still attached to > its adjacent insn and repositioned later? We're removing them with remove_notes, then we re-emit them with reemit_notes after scheduling their adjacent insns (either one is happening roughly once per a region), and only then we do reposition_p_e_notes (once per a function). Looking closely, the latter will not even find the repositioned note as it is looking only in the EXIT's predecessors, and the ar.pfs insn got moved pretty high in the CFG. Andrey
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c index 900994d..cb3a21f 100644 --- a/gcc/dwarf2out.c +++ b/gcc/dwarf2out.c @@ -790,8 +790,9 @@ dwarf2out_cfi_label (bool force) } else { - ASM_GENERATE_INTERNAL_LABEL (label, "LCFI", dwarf2out_cfi_label_num++); - ASM_OUTPUT_LABEL (asm_out_file, label); + int num = dwarf2out_cfi_label_num++; + ASM_GENERATE_INTERNAL_LABEL (label, "LCFI", num); + ASM_OUTPUT_DEBUG_LABEL (asm_out_file, "LCFI", num); } return label;