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

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.


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
>>
>>

On 11/06/14 06:01, Evgeny Stupachenko 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.
OK for the trunk.  One more Darwin bug gets squashed :-)


jeff

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
>>>
>>>

Thanks.
Committed to trunk with that fix:

Author: kyukhin
Date: Fri Nov  7 20:42:36 2014
New Revision: 217237

URL: https://gcc.gnu.org/viewcvs?rev=217237&root=gcc&view=rev
Log:
PR target/63534

gcc/
        * config/i386/i386.md (builtin_setjmp_receiver): Use
        pic_offset_table_rtx for PIC register.
        (nonlocal_goto_receiver): Delete.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.md

On Fri, Nov 7, 2014 at 11:33 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> 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.

Hi Jeff,

On 7 Nov 2014, at 20:28, Jeff Law wrote:

> On 11/06/14 06:01, Evgeny Stupachenko 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.
> OK for the trunk.  One more Darwin bug gets squashed :-)

FOAD, are you referring to this bug (63534) - or is there an implication that the nonlocal_goto_reciever was buggy (*before* the trunk changes) - I ask because the same implementation is present on the open branches, and if it's broken would like to sort that out.
cheers
Iain

On 11/08/14 09:16, Iain Sandoe wrote:
> Hi Jeff,
>
> On 7 Nov 2014, at 20:28, Jeff Law wrote:
>
>> On 11/06/14 06:01, Evgeny Stupachenko 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.
>> OK for the trunk.  One more Darwin bug gets squashed :-)
>
> FOAD, are you referring to this bug (63534) - or is there an
> implication that the nonlocal_goto_reciever was buggy (*before* the
> trunk changes) - I ask because the same implementation is present on
> the open branches, and if it's broken would like to sort that out.
> cheers Iain
No need to do anything on the branches as the issue with the 
{setjmp,nonlocal_goto}_receiver is specific to the change to expose the 
PIC register to register allocation.

jeff