On Mon, 2 Sep 2019 at 18:12, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Sorry for the slow reply.
>
> Christophe Lyon <christophe.lyon@st.com> writes:
> > On 16/07/2019 13:58, Richard Sandiford wrote:
> >> Christophe Lyon <christophe.lyon@st.com> writes:
> >>> +(define_insn "*restore_pic_register_after_call"
> >>> +  [(parallel [(unspec [(match_operand:SI 0 "s_register_operand" "=r,r")
> >>> +                  (match_operand:SI 1 "nonimmediate_operand" "r,m")]
> >>> +          UNSPEC_PIC_RESTORE)
> >>> +         (use (match_dup 1))
> >>> +         (clobber (match_dup 0))])
> >>> +  ]
> >>> +  ""
> >>> +  "@
> >>> +  mov\t%0, %1
> >>> +  ldr\t%0, %1"
> >>> +)
> >>> +
> >>>   (define_expand "call_internal"
> >>>     [(parallel [(call (match_operand 0 "memory_operand" "")
> >>>                 (match_operand 1 "general_operand" ""))
> >>
> >> Since operand 0 is significant after the instruction, I think this
> >> should be:
> >>
> >> (define_insn "*restore_pic_register_after_call"
> >>    [(set (match_operand:SI 0 "s_register_operand" "+r,r")
> >>      (unspec:SI [(match_dup 0)
> >>                  (match_operand:SI 1 "nonimmediate_operand" "r,m")]
> >>                 UNSPEC_PIC_RESTORE))]
> >>    ...
> >>
> >> The (use (match_dup 1)) looks redundant, since the unspec itself
> >> uses operand 1.
> >>
> > When I try that, I have cases where the restore instruction is discarded, when the call happens just before function return. Since r9 is caller-saved, it should be restored but after dse2 the dumps say:
> > (insn (set (reg:SI 9 r9)
> >     (unspec:SI [
> >         (reg:SI 9 r9)
> >         (reg:SI 4 r4 [121])
> >       ] UNSPEC_PIC_RESTORE))
> > (expr_list:REG_UNUSED (reg:SI 9 r9) (nil))))
> >
> > and this is later removed by cprop_hardreg (which says the exit block uses r4, sp, and lr: should I make it use r9?)
>
> But if it's caller-saved (i.e. call-clobbered), function A shouldn't
> need to restore r9 after a call unless A needs the value of r9 for
> something.  I.e. A shouldn't need to restore r9 for A's own caller,
> because the caller should be doing that iself.
>
> So if r9 is caller-saved and not referenced between the call and
> function exit, deleting the restore sounds like the right thing to do.
>

Of course! I should have found that myself: I tried this change before I removed
an "optimization" we had that avoided restoring  r9 before calling
functions in the same module... thus breaking the ABI.

Since the previous patch I send didn't have this "optimization", the
above now works.
New patch attached.

Thanks!

Christophe


> Richard