Patchwork [RFC/CFT] Hookize TARGET_UNWIND_INFO and related macros

login
register
mail settings
Submitter Richard Henderson
Date Sept. 27, 2010, 10:26 p.m.
Message ID <4CA11A25.5090901@redhat.com>
Download mbox | patch
Permalink /patch/65922/
State New
Headers show

Comments

Richard Henderson - Sept. 27, 2010, 10:26 p.m.
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~

        * dwarf2out.c (dwarf2out_cfi_label): Use ASM_OUTPUT_DEBUG_LABEL.
Steve Ellcey - Sept. 28, 2010, 6:42 p.m.
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
Andrey Belevantsev - Sept. 30, 2010, 12:46 p.m.
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");
}
Richard Henderson - Sept. 30, 2010, 4:24 p.m.
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~
Andrey Belevantsev - Oct. 1, 2010, 9:46 a.m.
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
Richard Henderson - Oct. 1, 2010, 3:06 p.m.
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~
Andrey Belevantsev - Oct. 4, 2010, 8:22 a.m.
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

Patch

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;