Re: Add unwind information to mips epilogues

[Richard Sandiford <rdsandiford at googlemail dot com>]

Bernd Schmidt <bernds@codesourcery.com> writes:
> On 09/08/11 16:08, Richard Sandiford wrote:
>> I suppose I still don't get why this is OK but this:
>> 
>>> @@ -10324,12 +10350,26 @@ mips_expand_epilogue (bool sibcall_p)
>>>        if (!TARGET_MIPS16)
>>>  	target = stack_pointer_rtx;
>>>  
>>> -      emit_insn (gen_add3_insn (target, base, adjust));
>>> +      insn = emit_insn (gen_add3_insn (target, base, adjust));
>>> +      if (!frame_pointer_needed && target == stack_pointer_rtx)
>>> +	{
>>> +	  RTX_FRAME_RELATED_P (insn) = 1;
>>> +	  add_reg_note (insn, REG_CFA_DEF_CFA,
>>> +			plus_constant (stack_pointer_rtx, step2));
>>> +	}
>> 
>> triggered ICEs without the !frame_pointer_required.  I think I need
>> to play around with it a bit before I understand enough to review.
>> I'll try to find time this weekend.
>
> I've rebuilt and tested it again (shrink-wrapping included, not sure if
> it makes a difference). What seems to happen is that we have
>
> (insn/f 60 59 61 2 (set (reg/f:SI 30 $fp) (reg/f:SI 29 $sp))
>
> (note 61 60 8 2 NOTE_INSN_PROLOGUE_END)
>
> (jump_insn 8 61 49 2 (set (pc)
>         (if_then_else (eq (reg/v:SI 4 $4 [orig:196 b ] [196])
>                 (reg:SI 2 $2 [198]))
>             (label_ref:SI 29) (pc)))
>
> [...]
>
> (jump_insn 10 9 48 3 (set (pc)
>         (if_then_else (eq (reg/v:SI 4 $4 [orig:196 b ] [196])
>                 (reg:SI 2 $2 [199]))
>             (label_ref:SI 29) (pc)))
>
> (note 48 10 62 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
>
> (note 62 48 63 4 NOTE_INSN_EPILOGUE_BEG)
>
> (insn/f 63 62 64 4 (set (reg/f:SI 29 $sp) (reg/f:SI 30 $fp))
>      (REG_CFA_DEF_CFA (plus:SI (reg/f:SI 29 $sp) (const_int 8))
>
> and reorg.c manages to hoist insn 63 into a delay slot for jump_insn 10.
> Presumably it saw insn 60 somehow? In any case, we can now arrive at
> label 29 with either FP or SP as CFA reg.
>
> This is gcc.dg/pr49994-1.c.

Thanks.  I suppose the transformation would be valid if it really had
seen insn 60, but it looks like:

(insn 39 38 40 (set (reg/f:SI 29 $sp)
        (mem:SI (plus:SI (reg/f:SI 15 $15 [orig:197 CHAIN.1 ] [197])
                (const_int 4 [0x4])) [0 S4 A32])) ../../../gcc/gcc/testsuite/gcc.dg/pr49994-1.c:14 278 {*movsi_internal}
     (expr_list:REG_DEAD (reg/f:SI 15 $15 [orig:197 CHAIN.1 ] [197])
        (nil)))

stops resource.c from considering the stack pointer to be live on
fallthrough, and so reorg.c is simply speculating a move to what
it thinks is a dead register.  Which seems like a bug.

(It's probably safe in this example, but we haven't done the work to
prove that.  In particular, we haven't proven that the MEM in insn 39
isn't an access to the current frame.)

But like you say, reorg.c does analyse register contents to some extent,
and it's hard to prove that it wouldn't be able to pull the same trick
for valid reasons.  Should we simply forbid speculation of frame-related
instructions?  E.g. by replacing the may_trap_or_fault_p calls with something
that also checks RTX_FRAME_RELATED_P?  (We'd still be able to use frame-
related insns for annulled and always-true branches.)  Or is that too
big a hammer?

Richard