0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[bin.cheng]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

Hi,
I ran into a wrong code bug in code with deep template instantiation when working on sdx::simd.
The root cause as described in commit summary is we skip prologue insns in init_alias_analysis.
This simple patch fixes the issue, however, it's hard to reduce a case because of heavy use of
templates.
Bootstrap and test on x86_64, is it OK?

Thanks,
bin

[-- Attachment #2: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch --]
[-- Type: application/octet-stream, Size: 1344 bytes --]

Subject: [PATCH 1/2] Don't skip prologue instructions as it could affect alias
 analysis

In init_alias_analysis, we skip prologue instructions but this is
wrong as it could affect base value analysis for registers as well
as following analysis/transformations like cselib and dse:
  prologue:
    x12 = 0x1810
    sp = sp - x12
  ...
    ...
    x12 = SYMBOL_REF(.LC89)
Here reg_base_value[12] is set to ".LC89", however this is only true
after the second instruction setting x12.  The patch fixes the issue
by just handling prologue instructions as normal.  Though ideally it
can be improved in context-sensitive way.

2021-07-14  Bin Cheng  <bin.cheng@linux.alibaba.com>

        * alias.c (init_alias_analysis): Don't skip prologue insns.
---
 gcc/alias.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gcc/alias.c b/gcc/alias.c
index 69e1eb89ac6..9d5eeb082ee 100644
--- a/gcc/alias.c
+++ b/gcc/alias.c
@@ -3459,8 +3459,9 @@ init_alias_analysis (void)
 		{
 		  rtx note, set;
 
-		  if (could_be_prologue_epilogue
-		      && prologue_epilogue_contains (insn))
+		  /* Don't skip prologue insn because it could change
+		     reg_seen.  */
+		  if (could_be_prologue_epilogue && epilogue_contains (insn))
 		    continue;
 
 		  /* If this insn has a noalias note, process it,  Otherwise,
-- 
2.19.1.6.gb485710b

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Bin.Cheng]

Gentle ping.  Any suggestions would be appreciated.

Thanks,
bin

On Wed, Jul 14, 2021 at 5:15 PM bin.cheng via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
> I ran into a wrong code bug in code with deep template instantiation when working on sdx::simd.
> The root cause as described in commit summary is we skip prologue insns in init_alias_analysis.
> This simple patch fixes the issue, however, it's hard to reduce a case because of heavy use of
> templates.
> Bootstrap and test on x86_64, is it OK?
>
> Thanks,
> bin

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Richard Biener]

On Thu, Jul 22, 2021 at 9:02 AM Bin.Cheng via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Gentle ping.  Any suggestions would be appreciated.

So just to say something - does the existing code mean that any use of
the alias info on prologue/epilogue insns is wrong?  We have

  /* The prologue/epilogue insns are not threaded onto the
     insn chain until after reload has completed.  Thus,
     there is no sense wasting time checking if INSN is in
     the prologue/epilogue until after reload has completed.  */
  bool could_be_prologue_epilogue = ((targetm.have_prologue ()
                                      || targetm.have_epilogue ())
                                     && reload_completed);

so when !could_be_prologue_epilogue then passes shouldn't run into
them if the comment is correct.  But else even epilogue stmts could appear
anywhere (like scheduled around)?  So why's skipping those OK?

Are passes supposed to check whether they are dealing with pro/epilogue
insns and not touch them?  CCing people that might know.

Richard.

> Thanks,
> bin
>
> On Wed, Jul 14, 2021 at 5:15 PM bin.cheng via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > Hi,
> > I ran into a wrong code bug in code with deep template instantiation when working on sdx::simd.
> > The root cause as described in commit summary is we skip prologue insns in init_alias_analysis.
> > This simple patch fixes the issue, however, it's hard to reduce a case because of heavy use of
> > templates.
> > Bootstrap and test on x86_64, is it OK?
> >
> > Thanks,
> > bin

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Segher Boessenkool]

Hi!

On Thu, Jul 22, 2021 at 03:04:32PM +0200, Richard Biener wrote:
> On Thu, Jul 22, 2021 at 9:02 AM Bin.Cheng via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > Gentle ping.  Any suggestions would be appreciated.

Bin: I never received your messages.  And my replies to you @alibaba are
refused by the mail system there as well.

I'll dig things up from the ML archives.

> So just to say something - does the existing code mean that any use of
> the alias info on prologue/epilogue insns is wrong?  We have
> 
>   /* The prologue/epilogue insns are not threaded onto the
>      insn chain until after reload has completed.  Thus,
>      there is no sense wasting time checking if INSN is in
>      the prologue/epilogue until after reload has completed.  */
>   bool could_be_prologue_epilogue = ((targetm.have_prologue ()
>                                       || targetm.have_epilogue ())
>                                      && reload_completed);
> 
> so when !could_be_prologue_epilogue then passes shouldn't run into
> them if the comment is correct.  But else even epilogue stmts could appear
> anywhere (like scheduled around)?  So why's skipping those OK?
> 
> Are passes supposed to check whether they are dealing with pro/epilogue
> insns and not touch them?  CCing people that might know.

*logue isns should not exist until pass pro_and_epilogue.  This pass is
later than reload.

All such stack accesses use gen_frame_mem(), which does
  set_mem_alias_set (mem, get_frame_alias_set ());
so what is going wrong here?


Segher

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Jeff Law]

On 7/22/2021 7:04 AM, Richard Biener via Gcc-patches wrote:
> On Thu, Jul 22, 2021 at 9:02 AM Bin.Cheng via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>> Gentle ping.  Any suggestions would be appreciated.
> So just to say something - does the existing code mean that any use of
> the alias info on prologue/epilogue insns is wrong?  We have
>
>    /* The prologue/epilogue insns are not threaded onto the
>       insn chain until after reload has completed.  Thus,
>       there is no sense wasting time checking if INSN is in
>       the prologue/epilogue until after reload has completed.  */
>    bool could_be_prologue_epilogue = ((targetm.have_prologue ()
>                                        || targetm.have_epilogue ())
>                                       && reload_completed);
>
> so when !could_be_prologue_epilogue then passes shouldn't run into
> them if the comment is correct.  But else even epilogue stmts could appear
> anywhere (like scheduled around)?  So why's skipping those OK?
These insns don't exist until after reload has completed.  I think this 
code is just trying to be more compile-time efficient and not look for 
them when they're known to not exist.

As for why they're skipped?  That seems wrong to me.  That was added by 
Kenner:

https://gcc.gnu.org/pipermail/gcc-patches/2000-May/031560.html

Interestingly enough myself (and others) preserved that behavior through 
several updates to this code.

>
> Are passes supposed to check whether they are dealing with pro/epilogue
> insns and not touch them?  CCing people that might know.
Generally most passes can treat them as any other RTL.

Jeff

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Segher Boessenkool]

On Fri, Jul 23, 2021 at 10:27:37AM -0600, Jeff Law wrote:
> On 7/22/2021 7:04 AM, Richard Biener via Gcc-patches wrote:
> >On Thu, Jul 22, 2021 at 9:02 AM Bin.Cheng via Gcc-patches
> ><gcc-patches@gcc.gnu.org> wrote:
> >>Gentle ping.  Any suggestions would be appreciated.
> >So just to say something - does the existing code mean that any use of
> >the alias info on prologue/epilogue insns is wrong?  We have
> >
> >   /* The prologue/epilogue insns are not threaded onto the
> >      insn chain until after reload has completed.  Thus,
> >      there is no sense wasting time checking if INSN is in
> >      the prologue/epilogue until after reload has completed.  */
> >   bool could_be_prologue_epilogue = ((targetm.have_prologue ()
> >                                       || targetm.have_epilogue ())
> >                                      && reload_completed);
> >
> >so when !could_be_prologue_epilogue then passes shouldn't run into
> >them if the comment is correct.  But else even epilogue stmts could appear
> >anywhere (like scheduled around)?  So why's skipping those OK?
> These insns don't exist until after reload has completed.  I think this 
> code is just trying to be more compile-time efficient and not look for 
> them when they're known to not exist.

Yeah.  Unfortunately it isn't trivial to see if this is a premature
optimisation, or if this is needed for correctness instead.  But it is
stage 1 still :-)

> >Are passes supposed to check whether they are dealing with pro/epilogue
> >insns and not touch them?  CCing people that might know.
> Generally most passes can treat them as any other RTL.

Yup, if the *logues are just RTL, that RTL follows just the normal
semantics of RTL.  This means that dead code can be deleted, etc.  Well
that is about the most interesting transform that can be done so late
in the compilation pipeline :-)


Segher

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Jeff Law]

On 7/26/2021 4:55 PM, Segher Boessenkool wrote:
> On Fri, Jul 23, 2021 at 10:27:37AM -0600, Jeff Law wrote:
>> On 7/22/2021 7:04 AM, Richard Biener via Gcc-patches wrote:
>>> On Thu, Jul 22, 2021 at 9:02 AM Bin.Cheng via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>> Gentle ping.  Any suggestions would be appreciated.
>>> So just to say something - does the existing code mean that any use of
>>> the alias info on prologue/epilogue insns is wrong?  We have
>>>
>>>    /* The prologue/epilogue insns are not threaded onto the
>>>       insn chain until after reload has completed.  Thus,
>>>       there is no sense wasting time checking if INSN is in
>>>       the prologue/epilogue until after reload has completed.  */
>>>    bool could_be_prologue_epilogue = ((targetm.have_prologue ()
>>>                                        || targetm.have_epilogue ())
>>>                                       && reload_completed);
>>>
>>> so when !could_be_prologue_epilogue then passes shouldn't run into
>>> them if the comment is correct.  But else even epilogue stmts could appear
>>> anywhere (like scheduled around)?  So why's skipping those OK?
>> These insns don't exist until after reload has completed.  I think this
>> code is just trying to be more compile-time efficient and not look for
>> them when they're known to not exist.
> Yeah.  Unfortunately it isn't trivial to see if this is a premature
> optimisation, or if this is needed for correctness instead.  But it is
> stage 1 still :-)


https://gcc.gnu.org/pipermail/gcc-patches/2000-May/031560.html

Jeff

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Segher Boessenkool]

On Wed, Jul 14, 2021 at 05:14:16PM +0800, bin.cheng via Gcc-patches wrote:
> Hi,
> I ran into a wrong code bug in code with deep template instantiation when working on sdx::simd.
> The root cause as described in commit summary is we skip prologue insns in init_alias_analysis.
> This simple patch fixes the issue, however, it's hard to reduce a case because of heavy use of
> templates.

> Subject: [PATCH 1/2] Don't skip prologue instructions as it could affect alias
>  analysis
> 
> In init_alias_analysis, we skip prologue instructions but this is
> wrong as it could affect base value analysis for registers as well
> as following analysis/transformations like cselib and dse:
>   prologue:
>     x12 = 0x1810
>     sp = sp - x12
>   ...
>     ...
>     x12 = SYMBOL_REF(.LC89)
> Here reg_base_value[12] is set to ".LC89", however this is only true
> after the second instruction setting x12.  The patch fixes the issue
> by just handling prologue instructions as normal.  Though ideally it
> can be improved in context-sensitive way.

In what pass do you get the bad behaviour?  dse2?  postreload?  Or what
else?

Your patch looks correct, but I'd like to know why it has seemed to work
for so long :-)


Segher

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Bin.Cheng]

On Fri, Jul 23, 2021 at 7:53 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Wed, Jul 14, 2021 at 05:14:16PM +0800, bin.cheng via Gcc-patches wrote:
> > Hi,
> > I ran into a wrong code bug in code with deep template instantiation when working on sdx::simd.
> > The root cause as described in commit summary is we skip prologue insns in init_alias_analysis.
> > This simple patch fixes the issue, however, it's hard to reduce a case because of heavy use of
> > templates.
>
> > Subject: [PATCH 1/2] Don't skip prologue instructions as it could affect alias
> >  analysis
> >
> > In init_alias_analysis, we skip prologue instructions but this is
> > wrong as it could affect base value analysis for registers as well
> > as following analysis/transformations like cselib and dse:
> >   prologue:
> >     x12 = 0x1810
> >     sp = sp - x12
> >   ...
> >     ...
> >     x12 = SYMBOL_REF(.LC89)
> > Here reg_base_value[12] is set to ".LC89", however this is only true
> > after the second instruction setting x12.  The patch fixes the issue
> > by just handling prologue instructions as normal.  Though ideally it
> > can be improved in context-sensitive way.
>
> In what pass do you get the bad behaviour?  dse2?  postreload?  Or what
> else?
Hi Segher,
It is dse2 deleting non dead stores based on wrong cse info again
based on wrong alias info.
The code flow is quite tricky, given insn list like:
     x12 = 0x1810
     sp = sp - x12
     ...
     [sp + offset] = x
     y = [sp + offset]
     ...
     [sp + offset] = z
     ...
     x12 = SYMBOL_REF(.LC89)

1、alias computes wrong reg_base_value[12] = symbol_ref(.LC89)
2、when dse2 process "y = [sp + offset]", the calling sequence is :
      scan_insn
        -> check_mem_read_rtx
              -> check_mem_read_rtx
                    -> canon_true_dependence(group_id == -1)
                          -> true_dependence_1 which has below code:
------------------------------------------------------------------
  base = find_base_term (x_addr);
  if (base && (GET_CODE (base) == LABEL_REF
       || (GET_CODE (base) == SYMBOL_REF
   && CONSTANT_POOL_ADDRESS_P (base))))
    return 0;
x_addr is "sp - x12", sp has no base term, however x12 has
symbol_ref(.LC89) base term, and the code returns 0.  As a result,
dse2 considers storing x as dead when processing "[sp + offset] = z".

Sorry I can't reproduce a case out of it.

Thanks,
bin

>
> Your patch looks correct, but I'd like to know why it has seemed to work
> for so long :-)
>
>
> Segher

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Jeff Law]

On 7/14/2021 3:14 AM, bin.cheng via Gcc-patches wrote:
> Hi,
> I ran into a wrong code bug in code with deep template instantiation when working on sdx::simd.
> The root cause as described in commit summary is we skip prologue insns in init_alias_analysis.
> This simple patch fixes the issue, however, it's hard to reduce a case because of heavy use of
> templates.
> Bootstrap and test on x86_64, is it OK?
It's a clear correctness improvement, but what's unclear to me is why 
we'd want to skip them in the epilogue either.

Jeff

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Bin.Cheng]

On Sat, Jul 24, 2021 at 12:30 AM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 7/14/2021 3:14 AM, bin.cheng via Gcc-patches wrote:
> > Hi,
> > I ran into a wrong code bug in code with deep template instantiation when working on sdx::simd.
> > The root cause as described in commit summary is we skip prologue insns in init_alias_analysis.
> > This simple patch fixes the issue, however, it's hard to reduce a case because of heavy use of
> > templates.
> > Bootstrap and test on x86_64, is it OK?
> It's a clear correctness improvement, but what's unclear to me is why
> we'd want to skip them in the epilogue either.
I can only guess, there is nothing to initialize epilogue for because
no code follows.

Thanks,
bin
>
> Jeff

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Jeff Law]

On 7/25/2021 7:47 PM, Bin.Cheng wrote:
> On Sat, Jul 24, 2021 at 12:30 AM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>> On 7/14/2021 3:14 AM, bin.cheng via Gcc-patches wrote:
>>> Hi,
>>> I ran into a wrong code bug in code with deep template instantiation when working on sdx::simd.
>>> The root cause as described in commit summary is we skip prologue insns in init_alias_analysis.
>>> This simple patch fixes the issue, however, it's hard to reduce a case because of heavy use of
>>> templates.
>>> Bootstrap and test on x86_64, is it OK?
>> It's a clear correctness improvement, but what's unclear to me is why
>> we'd want to skip them in the epilogue either.
> I can only guess, there is nothing to initialize epilogue for because
> no code follows.
Yea, but couldn't the lack of analysis of the epilogue lead to a pass 
mis-optimizing code within the epilogue itself?  It's not terribly 
likely, but it just seems wrong to skip the epilogue like this.  
Remember, the aliasing bits are just an analysis phase to find the 
aliasing relationships that exist and we don't necessarily know how that 
data is going to be used.  It may in fact be safe now, but may not be 
safe in the future if someone added a late RTL pass that used the 
aliasing info in a new way.

The more I think about it, the more I think we should remove remove this 
hunk of code completely.  There is some chance for fallout, but I think 
it's unlikely.

Jeff

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Bin.Cheng]

On Mon, Jul 26, 2021 at 11:07 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 7/25/2021 7:47 PM, Bin.Cheng wrote:
> > On Sat, Jul 24, 2021 at 12:30 AM Jeff Law via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >>
> >> On 7/14/2021 3:14 AM, bin.cheng via Gcc-patches wrote:
> >>> Hi,
> >>> I ran into a wrong code bug in code with deep template instantiation when working on sdx::simd.
> >>> The root cause as described in commit summary is we skip prologue insns in init_alias_analysis.
> >>> This simple patch fixes the issue, however, it's hard to reduce a case because of heavy use of
> >>> templates.
> >>> Bootstrap and test on x86_64, is it OK?
> >> It's a clear correctness improvement, but what's unclear to me is why
> >> we'd want to skip them in the epilogue either.
> > I can only guess, there is nothing to initialize epilogue for because
> > no code follows.
> Yea, but couldn't the lack of analysis of the epilogue lead to a pass
> mis-optimizing code within the epilogue itself?  It's not terribly
> likely, but it just seems wrong to skip the epilogue like this.
> Remember, the aliasing bits are just an analysis phase to find the
> aliasing relationships that exist and we don't necessarily know how that
> data is going to be used.  It may in fact be safe now, but may not be
> safe in the future if someone added a late RTL pass that used the
> aliasing info in a new way.
>
> The more I think about it, the more I think we should remove remove this
> hunk of code completely.  There is some chance for fallout, but I think
> it's unlikely.
Hi Jeff,
Thanks for the suggestion, here is the simple patch removing all of it.
diff --git a/gcc/alias.c b/gcc/alias.c
index 69e1eb89ac6..099acabca6b 100644
--- a/gcc/alias.c
+++ b/gcc/alias.c
@@ -3406,14 +3406,6 @@ init_alias_analysis (void)
   rpo = XNEWVEC (int, n_basic_blocks_for_fn (cfun));
   rpo_cnt = pre_and_rev_post_order_compute (NULL, rpo, false);

-  /* The prologue/epilogue insns are not threaded onto the
-     insn chain until after reload has completed.  Thus,
-     there is no sense wasting time checking if INSN is in
-     the prologue/epilogue until after reload has completed.  */
-  bool could_be_prologue_epilogue = ((targetm.have_prologue ()
-                                     || targetm.have_epilogue ())
-                                    && reload_completed);
-
   pass = 0;
   do
     {
@@ -3459,10 +3451,6 @@ init_alias_analysis (void)
                {
                  rtx note, set;

-                 if (could_be_prologue_epilogue
-                     && prologue_epilogue_contains (insn))
-                   continue;
-
                  /* If this insn has a noalias note, process it,  Otherwise,
                     scan for sets.  A simple set will have no side effects
                     which could change the base value of any other
register.  */

No fallouts in bootstrap/test on x86_64.  Is it OK?

Thanks,
bin
>
> Jeff
>

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Richard Biener]

On Tue, Jul 27, 2021 at 10:51 AM Bin.Cheng via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Mon, Jul 26, 2021 at 11:07 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
> >
> >
> >
> > On 7/25/2021 7:47 PM, Bin.Cheng wrote:
> > > On Sat, Jul 24, 2021 at 12:30 AM Jeff Law via Gcc-patches
> > > <gcc-patches@gcc.gnu.org> wrote:
> > >>
> > >>
> > >> On 7/14/2021 3:14 AM, bin.cheng via Gcc-patches wrote:
> > >>> Hi,
> > >>> I ran into a wrong code bug in code with deep template instantiation when working on sdx::simd.
> > >>> The root cause as described in commit summary is we skip prologue insns in init_alias_analysis.
> > >>> This simple patch fixes the issue, however, it's hard to reduce a case because of heavy use of
> > >>> templates.
> > >>> Bootstrap and test on x86_64, is it OK?
> > >> It's a clear correctness improvement, but what's unclear to me is why
> > >> we'd want to skip them in the epilogue either.
> > > I can only guess, there is nothing to initialize epilogue for because
> > > no code follows.
> > Yea, but couldn't the lack of analysis of the epilogue lead to a pass
> > mis-optimizing code within the epilogue itself?  It's not terribly
> > likely, but it just seems wrong to skip the epilogue like this.
> > Remember, the aliasing bits are just an analysis phase to find the
> > aliasing relationships that exist and we don't necessarily know how that
> > data is going to be used.  It may in fact be safe now, but may not be
> > safe in the future if someone added a late RTL pass that used the
> > aliasing info in a new way.
> >
> > The more I think about it, the more I think we should remove remove this
> > hunk of code completely.  There is some chance for fallout, but I think
> > it's unlikely.
> Hi Jeff,
> Thanks for the suggestion, here is the simple patch removing all of it.
> diff --git a/gcc/alias.c b/gcc/alias.c
> index 69e1eb89ac6..099acabca6b 100644
> --- a/gcc/alias.c
> +++ b/gcc/alias.c
> @@ -3406,14 +3406,6 @@ init_alias_analysis (void)
>    rpo = XNEWVEC (int, n_basic_blocks_for_fn (cfun));
>    rpo_cnt = pre_and_rev_post_order_compute (NULL, rpo, false);
>
> -  /* The prologue/epilogue insns are not threaded onto the
> -     insn chain until after reload has completed.  Thus,
> -     there is no sense wasting time checking if INSN is in
> -     the prologue/epilogue until after reload has completed.  */
> -  bool could_be_prologue_epilogue = ((targetm.have_prologue ()
> -                                     || targetm.have_epilogue ())
> -                                    && reload_completed);
> -
>    pass = 0;
>    do
>      {
> @@ -3459,10 +3451,6 @@ init_alias_analysis (void)
>                 {
>                   rtx note, set;
>
> -                 if (could_be_prologue_epilogue
> -                     && prologue_epilogue_contains (insn))
> -                   continue;
> -
>                   /* If this insn has a noalias note, process it,  Otherwise,
>                      scan for sets.  A simple set will have no side effects
>                      which could change the base value of any other
> register.  */
>
> No fallouts in bootstrap/test on x86_64.  Is it OK?

OK.

Thanks,
Richard.

> Thanks,
> bin
> >
> > Jeff
> >

Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Jeff Law]

On 7/27/2021 2:50 AM, Bin.Cheng wrote:
> On Mon, Jul 26, 2021 at 11:07 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>
>>
>> On 7/25/2021 7:47 PM, Bin.Cheng wrote:
>>> On Sat, Jul 24, 2021 at 12:30 AM Jeff Law via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>> On 7/14/2021 3:14 AM, bin.cheng via Gcc-patches wrote:
>>>>> Hi,
>>>>> I ran into a wrong code bug in code with deep template instantiation when working on sdx::simd.
>>>>> The root cause as described in commit summary is we skip prologue insns in init_alias_analysis.
>>>>> This simple patch fixes the issue, however, it's hard to reduce a case because of heavy use of
>>>>> templates.
>>>>> Bootstrap and test on x86_64, is it OK?
>>>> It's a clear correctness improvement, but what's unclear to me is why
>>>> we'd want to skip them in the epilogue either.
>>> I can only guess, there is nothing to initialize epilogue for because
>>> no code follows.
>> Yea, but couldn't the lack of analysis of the epilogue lead to a pass
>> mis-optimizing code within the epilogue itself?  It's not terribly
>> likely, but it just seems wrong to skip the epilogue like this.
>> Remember, the aliasing bits are just an analysis phase to find the
>> aliasing relationships that exist and we don't necessarily know how that
>> data is going to be used.  It may in fact be safe now, but may not be
>> safe in the future if someone added a late RTL pass that used the
>> aliasing info in a new way.
>>
>> The more I think about it, the more I think we should remove remove this
>> hunk of code completely.  There is some chance for fallout, but I think
>> it's unlikely.
> Hi Jeff,
> Thanks for the suggestion, here is the simple patch removing all of it.
> diff --git a/gcc/alias.c b/gcc/alias.c
> index 69e1eb89ac6..099acabca6b 100644
> --- a/gcc/alias.c
> +++ b/gcc/alias.c
> @@ -3406,14 +3406,6 @@ init_alias_analysis (void)
>     rpo = XNEWVEC (int, n_basic_blocks_for_fn (cfun));
>     rpo_cnt = pre_and_rev_post_order_compute (NULL, rpo, false);
>
> -  /* The prologue/epilogue insns are not threaded onto the
> -     insn chain until after reload has completed.  Thus,
> -     there is no sense wasting time checking if INSN is in
> -     the prologue/epilogue until after reload has completed.  */
> -  bool could_be_prologue_epilogue = ((targetm.have_prologue ()
> -                                     || targetm.have_epilogue ())
> -                                    && reload_completed);
> -
>     pass = 0;
>     do
>       {
> @@ -3459,10 +3451,6 @@ init_alias_analysis (void)
>                  {
>                    rtx note, set;
>
> -                 if (could_be_prologue_epilogue
> -                     && prologue_epilogue_contains (insn))
> -                   continue;
> -
>                    /* If this insn has a noalias note, process it,  Otherwise,
>                       scan for sets.  A simple set will have no side effects
>                       which could change the base value of any other
> register.  */
>
> No fallouts in bootstrap/test on x86_64.  Is it OK?
Yes.  Go ahead and commit it.

Thanks for your patience,

Jeff