Re: [PARCH 1/2, x86, PR63534] Fix darwin bootstrap

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Thu, Nov 6, 2014 at 5:01 AM, Evgeny Stupachenko <evstupac@gmail.com> wrote:
> Now I see that equiv reload could be special for PIC register. Let's
> apply more conservative patch.
>
> Darwin bootstrap passed with the patch applied on r216304 (along with
> already committed to trunk patches from PR63618 and PR63620).
>
> 2014-11-06  Evgeny Stupachenko  <evstupac@gmail.com>
>
>         PR target/63534
>         * config/i386/i386.c (builtin_setjmp_receiver): Use pic_offset_table_rtx
>         for PIC register.
>         (nonlocal_goto_receiver): Delete.

It should be config/i386/i386.md, not config/i386/i386.c.

>
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index 7b1dd79..0df66ea 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -16989,10 +16989,9 @@
>    if (TARGET_MACHO)
>      {
>        rtx xops[3];
> -      rtx picreg = gen_rtx_REG (Pmode, PIC_OFFSET_TABLE_REGNUM);
>        rtx_code_label *label_rtx = gen_label_rtx ();
>        emit_insn (gen_set_got_labelled (pic_offset_table_rtx, label_rtx));
> -      xops[0] = xops[1] = picreg;
> +      xops[0] = xops[1] = pic_offset_table_rtx;
>        xops[2] = machopic_gen_offset (gen_rtx_LABEL_REF (SImode, label_rtx));
>        ix86_expand_binary_operator (MINUS, SImode, xops);
>      }
> @@ -17002,36 +17001,6 @@
>    DONE;
>  })
>
> -(define_insn_and_split "nonlocal_goto_receiver"
> -  [(unspec_volatile [(const_int 0)] UNSPECV_NLGR)]
> -  "TARGET_MACHO && !TARGET_64BIT && flag_pic"
> -  "#"
> -  "&& reload_completed"
> -  [(const_int 0)]
> -{
> -  if (crtl->uses_pic_offset_table)
> -    {
> -      rtx xops[3];
> -      rtx label_rtx = gen_label_rtx ();
> -      rtx tmp;
> -
> -      /* Get a new pic base.  */
> -      emit_insn (gen_set_got_labelled (pic_offset_table_rtx, label_rtx));
> -      /* Correct this with the offset from the new to the old.  */
> -      xops[0] = xops[1] = pic_offset_table_rtx;
> -      label_rtx = gen_rtx_LABEL_REF (SImode, label_rtx);
> -      tmp = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, label_rtx),
> -                           UNSPEC_MACHOPIC_OFFSET);
> -      xops[2] = gen_rtx_CONST (Pmode, tmp);
> -      ix86_expand_binary_operator (MINUS, SImode, xops);
> -    }
> -  else
> -    /* No pic reg restore needed.  */
> -    emit_note (NOTE_INSN_DELETED);
> -
> -  DONE;
> -})
> -
>  ;; Avoid redundant prefixes by splitting HImode arithmetic to SImode.
>  ;; Do not split instructions with mask registers.
>  (define_split
>
> On Wed, Nov 5, 2014 at 3:59 PM, Evgeny Stupachenko <evstupac@gmail.com> wrote:
>> On Tue, Nov 4, 2014 at 1:40 AM, Jeff Law <law@redhat.com> wrote:
>>> On 11/01/14 06:39, Evgeny Stupachenko wrote:
>>>>
>>>> When PIC register is pseudo there is nothing special about it's value
>>>> that setjmp can hurt. So if the pseudo register lives across
>>>> setjmp_receiver RA should care about correct allocation (in case it is
>>>> not saved/restored, it should go on stack). gcc.dg tests and specs
>>>> I've tested behave like this.
>>>
>>> If the allocator picked a call-clobbered register for the PIC register, then
>>> we're obviously OK since the setjmp has to be expected to clobber the PIC
>>> register.
>>>
>>> But if the PIC register is in a call-saved register, then it's going to be
>>> assumed to not be clobbered across calls and I don't believe that is
>>> guaranteed for builtin setjmp/longjmp.  Those restore SP, FP and an ARGP,
>>> but not anything else by default.
>>
>> I still don't see what is special for PIC register here. PIC pseudo
>> now behave as every other pseudo register.
>> If we assume that setjmp can change a pseudo register value we need
>> IRA/LRA "magic" for each pseudo register.
>>
>> I believe that when we had EBX fixed, IRA/LRA don't save/restore it anywhere.
>> Therefore we had to care about EBX value in special cases like
>> setjmp/non-local goto.
>> Now RA cares about PIC pseudo as well as about correct allocation for
>> any pseudo register.
>>
>>>
>>> So the callee might have clobbered the call saved hard register, expecting
>>> to restore its value in its epilogue.  But due to the longjmp, that epilogue
>>> never gets called and thus the call-saved register won't have the right
>>> value in the receiver.
>>>
>>> Now if your argument is that IRA/LRA handle this, that's fine, a pointer to
>>> that code would be appreciated so that it can be quickly audited. Certainly
>>> the old local-alloc/global-alloc had magic for setjmp/longjmp and maybe
>>> IRA/LRA does too, but it's better to be sure than just assume.
>>>
>>>>
>>>> The initial problem comes from non-local goto as it tries to emit
>>>> pseudo PIC register after reload.
>>>
>>> ?  You mean it emits a reference to the pseudo into RTL?  That would
>>> indicate that the allocators never put the pseudo into a hard register?!?
>>> RTL dumps with a few pointers to key insns would help here.
>>
>> Correct, that is why Darwin crashes with ICE on non-local goto.
>> We still have:
>>
>> (define_insn_and_split "nonlocal_goto_receiver"
>>   [(unspec_volatile [(const_int 0)] UNSPECV_NLGR)]
>>   "TARGET_MACHO && !TARGET_64BIT && flag_pic"
>>   "#"
>>   "&& reload_completed"
>>   [(const_int 0)]
>> {
>>   if (crtl->uses_pic_offset_table)
>>     {
>>       rtx xops[3];
>>       rtx label_rtx = gen_label_rtx ();
>>       rtx tmp;
>>
>>       /* Get a new pic base.  */
>>       emit_insn (gen_set_got_labelled (pic_offset_table_rtx, label_rtx));
>> .....
>> Here for MAC only we are trying to use pseudo PIC:
>> pic_offset_table_rtx when reload_completed.
>>
>>>
>>> jeff
>>>
>>>



-- 
H.J.