[PATCH, rs6000] Fix alias set of link reg save MEM

[PATCH, rs6000] Fix alias set of link reg save MEM

[Pat Haugen]

Make sure link reg save MEM has frame alias set, to match other link reg
save/restore code.

Bootstrap/regtest on powerpc64/powerpc64le with no new regressions. Ok for
trunk?

-Pat


2021-06-01  Pat Haugen  <pthaugen@linux.ibm.com>

gcc/ChangeLog:

	* config/rs6000/rs6000-logue.c (rs6000_emit_prologue): Use
	gen_frame_store.



diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
index 13c00e740d6..07337c4836a 100644
--- a/gcc/config/rs6000/rs6000-logue.c
+++ b/gcc/config/rs6000/rs6000-logue.c
@@ -3257,7 +3257,7 @@ rs6000_emit_prologue (void)
   if (!WORLD_SAVE_P (info) && info->lr_save_p
       && !cfun->machine->lr_is_wrapped_separately)
     {
-      rtx addr, reg, mem;
+      rtx reg;
 
       reg = gen_rtx_REG (Pmode, 0);
       START_USE (0);
@@ -3267,13 +3267,8 @@ rs6000_emit_prologue (void)
       if (!(strategy & (SAVE_NOINLINE_GPRS_SAVES_LR
 			| SAVE_NOINLINE_FPRS_SAVES_LR)))
 	{
-	  addr = gen_rtx_PLUS (Pmode, frame_reg_rtx,
-			       GEN_INT (info->lr_save_offset + frame_off));
-	  mem = gen_rtx_MEM (Pmode, addr);
-	  /* This should not be of rs6000_sr_alias_set, because of
-	     __builtin_return_address.  */
-
-	  insn = emit_move_insn (mem, reg);
+	  insn = emit_insn (gen_frame_store (reg, frame_reg_rtx,
+					     info->lr_save_offset + frame_off));
 	  rs6000_frame_related (insn, frame_reg_rtx, sp_off - frame_off,
 				NULL_RTX, NULL_RTX);
 	  END_USE (0);

Re: [PATCH, rs6000] Fix alias set of link reg save MEM

[Segher Boessenkool]

On Tue, Jun 01, 2021 at 03:34:50PM -0500, Pat Haugen wrote:
> Make sure link reg save MEM has frame alias set, to match other link reg
> save/restore code.

Okay for trunk and any backports (please do at least GCC 11).  Thanks!


Segher

Re: [PATCH, rs6000] Fix alias set of link reg save MEM

[Richard Biener]

On Tue, Jun 1, 2021 at 10:37 PM Pat Haugen via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Make sure link reg save MEM has frame alias set, to match other link reg
> save/restore code.
>
> Bootstrap/regtest on powerpc64/powerpc64le with no new regressions. Ok for
> trunk?
>
> -Pat
>
>
> 2021-06-01  Pat Haugen  <pthaugen@linux.ibm.com>
>
> gcc/ChangeLog:
>
>         * config/rs6000/rs6000-logue.c (rs6000_emit_prologue): Use
>         gen_frame_store.
>
>
>
> diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
> index 13c00e740d6..07337c4836a 100644
> --- a/gcc/config/rs6000/rs6000-logue.c
> +++ b/gcc/config/rs6000/rs6000-logue.c
> @@ -3257,7 +3257,7 @@ rs6000_emit_prologue (void)
>    if (!WORLD_SAVE_P (info) && info->lr_save_p
>        && !cfun->machine->lr_is_wrapped_separately)
>      {
> -      rtx addr, reg, mem;
> +      rtx reg;
>
>        reg = gen_rtx_REG (Pmode, 0);
>        START_USE (0);
> @@ -3267,13 +3267,8 @@ rs6000_emit_prologue (void)
>        if (!(strategy & (SAVE_NOINLINE_GPRS_SAVES_LR
>                         | SAVE_NOINLINE_FPRS_SAVES_LR)))
>         {
> -         addr = gen_rtx_PLUS (Pmode, frame_reg_rtx,
> -                              GEN_INT (info->lr_save_offset + frame_off));
> -         mem = gen_rtx_MEM (Pmode, addr);
> -         /* This should not be of rs6000_sr_alias_set, because of
> -            __builtin_return_address.  */

I can't figure what this comment meant - did you?  Note the old code
looks like it would end up with alias-set zero and thus more conservative
than with using frame-alias-set so this is an optimization?

Thanks,
Richard.

> -         insn = emit_move_insn (mem, reg);
> +         insn = emit_insn (gen_frame_store (reg, frame_reg_rtx,
> +                                            info->lr_save_offset + frame_off));
>           rs6000_frame_related (insn, frame_reg_rtx, sp_off - frame_off,
>                                 NULL_RTX, NULL_RTX);
>           END_USE (0);

Re: [PATCH, rs6000] Fix alias set of link reg save MEM

[Pat Haugen]

On 6/2/21 1:51 AM, Richard Biener wrote:
> On Tue, Jun 1, 2021 at 10:37 PM Pat Haugen via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> Make sure link reg save MEM has frame alias set, to match other link reg
>> save/restore code.
>>
>> Bootstrap/regtest on powerpc64/powerpc64le with no new regressions. Ok for
>> trunk?
>>
>> -Pat
>>
>>
>> 2021-06-01  Pat Haugen  <pthaugen@linux.ibm.com>
>>
>> gcc/ChangeLog:
>>
>>         * config/rs6000/rs6000-logue.c (rs6000_emit_prologue): Use
>>         gen_frame_store.
>>
>>
>>
>> diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
>> index 13c00e740d6..07337c4836a 100644
>> --- a/gcc/config/rs6000/rs6000-logue.c
>> +++ b/gcc/config/rs6000/rs6000-logue.c
>> @@ -3257,7 +3257,7 @@ rs6000_emit_prologue (void)
>>    if (!WORLD_SAVE_P (info) && info->lr_save_p
>>        && !cfun->machine->lr_is_wrapped_separately)
>>      {
>> -      rtx addr, reg, mem;
>> +      rtx reg;
>>
>>        reg = gen_rtx_REG (Pmode, 0);
>>        START_USE (0);
>> @@ -3267,13 +3267,8 @@ rs6000_emit_prologue (void)
>>        if (!(strategy & (SAVE_NOINLINE_GPRS_SAVES_LR
>>                         | SAVE_NOINLINE_FPRS_SAVES_LR)))
>>         {
>> -         addr = gen_rtx_PLUS (Pmode, frame_reg_rtx,
>> -                              GEN_INT (info->lr_save_offset + frame_off));
>> -         mem = gen_rtx_MEM (Pmode, addr);
>> -         /* This should not be of rs6000_sr_alias_set, because of
>> -            __builtin_return_address.  */
> 
> I can't figure what this comment meant - did you?  Note the old code
> looks like it would end up with alias-set zero and thus more conservative
> than with using frame-alias-set so this is an optimization?

No, I couldn't figure out the reasoning for the comment/code either. It's been that way since it was introduced in March 2000 as part of the “Merge in changes from newppc-branch.” patch. All other places where we save/restore the link reg use a MEM with frame-alias-set. This change is an optimization as you suspect in that it allows us to schedule non-aliased loads above the link reg store (which couldn't happen before due to use of alias-set zero).

-Pat

Re: [PATCH, rs6000] Fix alias set of link reg save MEM

[Richard Biener]

On Wed, Jun 2, 2021 at 1:15 PM Pat Haugen <pthaugen@linux.ibm.com> wrote:
>
> On 6/2/21 1:51 AM, Richard Biener wrote:
> > On Tue, Jun 1, 2021 at 10:37 PM Pat Haugen via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> Make sure link reg save MEM has frame alias set, to match other link reg
> >> save/restore code.
> >>
> >> Bootstrap/regtest on powerpc64/powerpc64le with no new regressions. Ok for
> >> trunk?
> >>
> >> -Pat
> >>
> >>
> >> 2021-06-01  Pat Haugen  <pthaugen@linux.ibm.com>
> >>
> >> gcc/ChangeLog:
> >>
> >>         * config/rs6000/rs6000-logue.c (rs6000_emit_prologue): Use
> >>         gen_frame_store.
> >>
> >>
> >>
> >> diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
> >> index 13c00e740d6..07337c4836a 100644
> >> --- a/gcc/config/rs6000/rs6000-logue.c
> >> +++ b/gcc/config/rs6000/rs6000-logue.c
> >> @@ -3257,7 +3257,7 @@ rs6000_emit_prologue (void)
> >>    if (!WORLD_SAVE_P (info) && info->lr_save_p
> >>        && !cfun->machine->lr_is_wrapped_separately)
> >>      {
> >> -      rtx addr, reg, mem;
> >> +      rtx reg;
> >>
> >>        reg = gen_rtx_REG (Pmode, 0);
> >>        START_USE (0);
> >> @@ -3267,13 +3267,8 @@ rs6000_emit_prologue (void)
> >>        if (!(strategy & (SAVE_NOINLINE_GPRS_SAVES_LR
> >>                         | SAVE_NOINLINE_FPRS_SAVES_LR)))
> >>         {
> >> -         addr = gen_rtx_PLUS (Pmode, frame_reg_rtx,
> >> -                              GEN_INT (info->lr_save_offset + frame_off));
> >> -         mem = gen_rtx_MEM (Pmode, addr);
> >> -         /* This should not be of rs6000_sr_alias_set, because of
> >> -            __builtin_return_address.  */
> >
> > I can't figure what this comment meant - did you?  Note the old code
> > looks like it would end up with alias-set zero and thus more conservative
> > than with using frame-alias-set so this is an optimization?
>
> No, I couldn't figure out the reasoning for the comment/code either. It's been that way since it was introduced in March 2000 as part of the “Merge in changes from newppc-branch.” patch. All other places where we save/restore the link reg use a MEM with frame-alias-set. This change is an optimization as you suspect in that it allows us to schedule non-aliased loads above the link reg store (which couldn't happen before due to use of alias-set zero).

So did you check the RTL (and alias-sets) produced by
__builtin_return_address?  Test coverage might
be low here and w/o scheduling opportunities to break things.

Richard.

> -Pat

Re: [PATCH, rs6000] Fix alias set of link reg save MEM

[Pat Haugen]

On 6/2/21 7:01 AM, Richard Biener wrote:
> On Wed, Jun 2, 2021 at 1:15 PM Pat Haugen <pthaugen@linux.ibm.com> wrote:
>>
>> On 6/2/21 1:51 AM, Richard Biener wrote:
>>> On Tue, Jun 1, 2021 at 10:37 PM Pat Haugen via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>> Make sure link reg save MEM has frame alias set, to match other link reg
>>>> save/restore code.
>>>>
>>>> Bootstrap/regtest on powerpc64/powerpc64le with no new regressions. Ok for
>>>> trunk?
>>>>
>>>> -Pat
>>>>
>>>>
>>>> 2021-06-01  Pat Haugen  <pthaugen@linux.ibm.com>
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>>         * config/rs6000/rs6000-logue.c (rs6000_emit_prologue): Use
>>>>         gen_frame_store.
>>>>
>>>>
>>>>
>>>> diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
>>>> index 13c00e740d6..07337c4836a 100644
>>>> --- a/gcc/config/rs6000/rs6000-logue.c
>>>> +++ b/gcc/config/rs6000/rs6000-logue.c
>>>> @@ -3257,7 +3257,7 @@ rs6000_emit_prologue (void)
>>>>    if (!WORLD_SAVE_P (info) && info->lr_save_p
>>>>        && !cfun->machine->lr_is_wrapped_separately)
>>>>      {
>>>> -      rtx addr, reg, mem;
>>>> +      rtx reg;
>>>>
>>>>        reg = gen_rtx_REG (Pmode, 0);
>>>>        START_USE (0);
>>>> @@ -3267,13 +3267,8 @@ rs6000_emit_prologue (void)
>>>>        if (!(strategy & (SAVE_NOINLINE_GPRS_SAVES_LR
>>>>                         | SAVE_NOINLINE_FPRS_SAVES_LR)))
>>>>         {
>>>> -         addr = gen_rtx_PLUS (Pmode, frame_reg_rtx,
>>>> -                              GEN_INT (info->lr_save_offset + frame_off));
>>>> -         mem = gen_rtx_MEM (Pmode, addr);
>>>> -         /* This should not be of rs6000_sr_alias_set, because of
>>>> -            __builtin_return_address.  */
>>>
>>> I can't figure what this comment meant - did you?  Note the old code
>>> looks like it would end up with alias-set zero and thus more conservative
>>> than with using frame-alias-set so this is an optimization?
>>
>> No, I couldn't figure out the reasoning for the comment/code either. It's been that way since it was introduced in March 2000 as part of the “Merge in changes from newppc-branch.” patch. All other places where we save/restore the link reg use a MEM with frame-alias-set. This change is an optimization as you suspect in that it allows us to schedule non-aliased loads above the link reg store (which couldn't happen before due to use of alias-set zero).
> 
> So did you check the RTL (and alias-sets) produced by
> __builtin_return_address?  Test coverage might
> be low here and w/o scheduling opportunities to break things.

__builtin_return_address creates it's own copy of the link reg to a pseudo upon function entry. It doesn't appear to try and "reuse" any LR copy/save location that might be generated via the prolog code. References to __builtin_return_address will then refer to that pseudo. So I don't see any connection between the prolog save code and __builtin_return_address.

-Pat

Re: [PATCH, rs6000] Fix alias set of link reg save MEM

[Segher Boessenkool]

On Wed, Jun 02, 2021 at 08:23:48AM -0500, Pat Haugen wrote:
> On 6/2/21 7:01 AM, Richard Biener wrote:
> > So did you check the RTL (and alias-sets) produced by
> > __builtin_return_address?  Test coverage might
> > be low here and w/o scheduling opportunities to break things.
> 
> __builtin_return_address creates it's own copy of the link reg to a pseudo upon function entry. It doesn't appear to try and "reuse" any LR copy/save location that might be generated via the prolog code. References to __builtin_return_address will then refer to that pseudo. So I don't see any connection between the prolog save code and __builtin_return_address.

That is for __builtin_return_address(0), the simple (and always working)
one.  It is harder for non-zero arguments (although I don't see why
those would not work, even with inlining).


Segher

Re: [PATCH, rs6000] Fix alias set of link reg save MEM

[Pat Haugen]

On 6/2/21 9:19 AM, Segher Boessenkool wrote:
> On Wed, Jun 02, 2021 at 08:23:48AM -0500, Pat Haugen wrote:
>> On 6/2/21 7:01 AM, Richard Biener wrote:
>>> So did you check the RTL (and alias-sets) produced by
>>> __builtin_return_address?  Test coverage might
>>> be low here and w/o scheduling opportunities to break things.
>>
>> __builtin_return_address creates it's own copy of the link reg to a pseudo upon function entry. It doesn't appear to try and "reuse" any LR copy/save location that might be generated via the prolog code. References to __builtin_return_address will then refer to that pseudo. So I don't see any connection between the prolog save code and __builtin_return_address.
> 
> That is for __builtin_return_address(0), the simple (and always working)
> one.  It is harder for non-zero arguments (although I don't see why
> those would not work, even with inlining).


Right, I realized after I sent the reply I was being a little too specific to __builtin_return_address(0). For non-zero args __builtin_return_address generates code to walk the appropriate number of stack frames back before loading the LR from the designated save area. All those mem references are using either frame-alias-set or 0 as their alias set, so we still should be fine. So regardless of the argument to the builtin, there is no connection between the current function's prologue LR save code and the code __builtin_return_address() would generate in the function.

-Pat