| Message ID | 000b01cf8ad5$f58b7be0$e0a273a0$@arm.com |
|---|---|
| State | New |
| Headers | show |
On 18/06/14 10:16, Terry Guo wrote: > > >> -----Original Message----- >> From: Richard Earnshaw >> Sent: Wednesday, June 18, 2014 4:31 PM >> To: Terry Guo >> Cc: gcc-patches@gcc.gnu.org; Ramana Radhakrishnan >> Subject: Re: [Patch, GCC/Thumb-1]Mishandle the label type insn in function >> thumb1_reorg >> >> On 10/06/14 12:42, Terry Guo wrote: >>> Hi There, >>> >>> The thumb1_reorg function use macro INSN_CODE to find expected >> instructions. >>> But the macro INSN_CODE doesn’t work for label type instruction. The >>> INSN_CODE(label_insn) will return the label number. When we have a lot >> of >>> labels and current label_insn is the first insn of basic block, the >>> INSN_CODE(label_insn) could accidentally equal to >> CODE_FOR_cbranchsi4_insn >>> in this case. This leads to ICE due to SET_SRC(label_insn) in subsequent >>> code. In general we should skip all such improper insns. This is the purpose >>> of attached small patch. >>> >>> Some failures in recent gcc regression test on thumb1 target are caused by >>> this reason. So with this patch, all of them passed and no new failures. Is >>> it ok to trunk? >>> >>> BR, >>> Terry >>> >>> 2014-06-10 Terry Guo <terry.guo@arm.com> >>> >>> * config/arm/arm.c (thumb1_reorg): Move to next basic block if the >> head >>> of current basic block isn’t a proper insn. >>> >> >> I think you should just test that "insn != BB_HEAD (bb)". The loop >> immediately above this deals with the !NON-DEBUG insns, so the logic is >> confusing the way you've written it. >> >> R. >> > > Thanks for comments. The patch is updated and tested. No more ICE. Is this one OK? > > BR, > Terry > Yes, this is fine. R. > > thumb1-reorg-v3.txt > > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c > index 85d2114..463707e 100644 > --- a/gcc/config/arm/arm.c > +++ b/gcc/config/arm/arm.c > @@ -16946,7 +16946,8 @@ thumb1_reorg (void) > insn = PREV_INSN (insn); > > /* Find the last cbranchsi4_insn in basic block BB. */ > - if (INSN_CODE (insn) != CODE_FOR_cbranchsi4_insn) > + if (insn == BB_HEAD (bb) > + || INSN_CODE (insn) != CODE_FOR_cbranchsi4_insn) > continue; > > /* Get the register with which we are comparing. */ >
On Wed, Jun 18, 2014 at 10:16 AM, Terry Guo <terry.guo@arm.com> wrote: > > >> -----Original Message----- >> From: Richard Earnshaw >> Sent: Wednesday, June 18, 2014 4:31 PM >> To: Terry Guo >> Cc: gcc-patches@gcc.gnu.org; Ramana Radhakrishnan >> Subject: Re: [Patch, GCC/Thumb-1]Mishandle the label type insn in function >> thumb1_reorg >> >> On 10/06/14 12:42, Terry Guo wrote: >> > Hi There, >> > >> > The thumb1_reorg function use macro INSN_CODE to find expected >> instructions. >> > But the macro INSN_CODE doesn't work for label type instruction. The >> > INSN_CODE(label_insn) will return the label number. When we have a lot >> of >> > labels and current label_insn is the first insn of basic block, the >> > INSN_CODE(label_insn) could accidentally equal to >> CODE_FOR_cbranchsi4_insn >> > in this case. This leads to ICE due to SET_SRC(label_insn) in subsequent >> > code. In general we should skip all such improper insns. This is the purpose >> > of attached small patch. >> > >> > Some failures in recent gcc regression test on thumb1 target are caused by >> > this reason. So with this patch, all of them passed and no new failures. Is >> > it ok to trunk? >> > >> > BR, >> > Terry >> > >> > 2014-06-10 Terry Guo <terry.guo@arm.com> >> > >> > * config/arm/arm.c (thumb1_reorg): Move to next basic block if the >> head >> > of current basic block isn't a proper insn. >> > >> >> I think you should just test that "insn != BB_HEAD (bb)". The loop >> immediately above this deals with the !NON-DEBUG insns, so the logic is >> confusing the way you've written it. >> >> R. >> > > Thanks for comments. The patch is updated and tested. No more ICE. Is this one OK? > > BR, > Terry Hi, The bug is also reported for 4.9 branch at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61712 As far as I checked, the ICE is also caused by label instruction as in this message thread. Since the fix is on trunk more than two weeks, could we back port it to 4.9 branch? Thanks, bin
On 04/07/14 10:36, Bin.Cheng wrote: > On Wed, Jun 18, 2014 at 10:16 AM, Terry Guo <terry.guo@arm.com> wrote: >> >> >>> -----Original Message----- >>> From: Richard Earnshaw >>> Sent: Wednesday, June 18, 2014 4:31 PM >>> To: Terry Guo >>> Cc: gcc-patches@gcc.gnu.org; Ramana Radhakrishnan >>> Subject: Re: [Patch, GCC/Thumb-1]Mishandle the label type insn in function >>> thumb1_reorg >>> >>> On 10/06/14 12:42, Terry Guo wrote: >>>> Hi There, >>>> >>>> The thumb1_reorg function use macro INSN_CODE to find expected >>> instructions. >>>> But the macro INSN_CODE doesn't work for label type instruction. The >>>> INSN_CODE(label_insn) will return the label number. When we have a lot >>> of >>>> labels and current label_insn is the first insn of basic block, the >>>> INSN_CODE(label_insn) could accidentally equal to >>> CODE_FOR_cbranchsi4_insn >>>> in this case. This leads to ICE due to SET_SRC(label_insn) in subsequent >>>> code. In general we should skip all such improper insns. This is the purpose >>>> of attached small patch. >>>> >>>> Some failures in recent gcc regression test on thumb1 target are caused by >>>> this reason. So with this patch, all of them passed and no new failures. Is >>>> it ok to trunk? >>>> >>>> BR, >>>> Terry >>>> >>>> 2014-06-10 Terry Guo <terry.guo@arm.com> >>>> >>>> * config/arm/arm.c (thumb1_reorg): Move to next basic block if the >>> head >>>> of current basic block isn't a proper insn. >>>> >>> >>> I think you should just test that "insn != BB_HEAD (bb)". The loop >>> immediately above this deals with the !NON-DEBUG insns, so the logic is >>> confusing the way you've written it. >>> >>> R. >>> >> >> Thanks for comments. The patch is updated and tested. No more ICE. Is this one OK? >> >> BR, >> Terry > > Hi, > The bug is also reported for 4.9 branch at > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61712 > As far as I checked, the ICE is also caused by label instruction as in > this message thread. > Since the fix is on trunk more than two weeks, could we back port it > to 4.9 branch? > > Thanks, > bin > Yes, unless an RM objects within 24 hours. R.
On Fri, Jul 4, 2014 at 10:36 AM, Bin.Cheng <amker.cheng@gmail.com> wrote: > On Wed, Jun 18, 2014 at 10:16 AM, Terry Guo <terry.guo@arm.com> wrote: >> >> >>> -----Original Message----- >>> From: Richard Earnshaw >>> Sent: Wednesday, June 18, 2014 4:31 PM >>> To: Terry Guo >>> Cc: gcc-patches@gcc.gnu.org; Ramana Radhakrishnan >>> Subject: Re: [Patch, GCC/Thumb-1]Mishandle the label type insn in function >>> thumb1_reorg >>> >>> On 10/06/14 12:42, Terry Guo wrote: >>> > Hi There, >>> > >>> > The thumb1_reorg function use macro INSN_CODE to find expected >>> instructions. >>> > But the macro INSN_CODE doesn't work for label type instruction. The >>> > INSN_CODE(label_insn) will return the label number. When we have a lot >>> of >>> > labels and current label_insn is the first insn of basic block, the >>> > INSN_CODE(label_insn) could accidentally equal to >>> CODE_FOR_cbranchsi4_insn >>> > in this case. This leads to ICE due to SET_SRC(label_insn) in subsequent >>> > code. In general we should skip all such improper insns. This is the purpose >>> > of attached small patch. >>> > >>> > Some failures in recent gcc regression test on thumb1 target are caused by >>> > this reason. So with this patch, all of them passed and no new failures. Is >>> > it ok to trunk? >>> > >>> > BR, >>> > Terry >>> > >>> > 2014-06-10 Terry Guo <terry.guo@arm.com> >>> > >>> > * config/arm/arm.c (thumb1_reorg): Move to next basic block if the >>> head >>> > of current basic block isn't a proper insn. >>> > >>> >>> I think you should just test that "insn != BB_HEAD (bb)". The loop >>> immediately above this deals with the !NON-DEBUG insns, so the logic is >>> confusing the way you've written it. >>> >>> R. >>> >> >> Thanks for comments. The patch is updated and tested. No more ICE. Is this one OK? >> >> BR, >> Terry > > Hi, > The bug is also reported for 4.9 branch at > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61712 > As far as I checked, the ICE is also caused by label instruction as in > this message thread. > Since the fix is on trunk more than two weeks, could we back port it > to 4.9 branch? Yes, since there have been no other issues reported. Can either of you also update PR61544 with the fields for Known to Work and Known to fail setting a target milestone of GCC 4.9.1 rather than leaving these fields empty. Ramana > > Thanks, > bin
On Fri, Jul 4, 2014 at 10:44 AM, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > On Fri, Jul 4, 2014 at 10:36 AM, Bin.Cheng <amker.cheng@gmail.com> wrote: >> On Wed, Jun 18, 2014 at 10:16 AM, Terry Guo <terry.guo@arm.com> wrote: >>> >>> >>>> -----Original Message----- >>>> From: Richard Earnshaw >>>> Sent: Wednesday, June 18, 2014 4:31 PM >>>> To: Terry Guo >>>> Cc: gcc-patches@gcc.gnu.org; Ramana Radhakrishnan >>>> Subject: Re: [Patch, GCC/Thumb-1]Mishandle the label type insn in function >>>> thumb1_reorg >>>> >>>> On 10/06/14 12:42, Terry Guo wrote: >>>> > Hi There, >>>> > >>>> > The thumb1_reorg function use macro INSN_CODE to find expected >>>> instructions. >>>> > But the macro INSN_CODE doesn't work for label type instruction. The >>>> > INSN_CODE(label_insn) will return the label number. When we have a lot >>>> of >>>> > labels and current label_insn is the first insn of basic block, the >>>> > INSN_CODE(label_insn) could accidentally equal to >>>> CODE_FOR_cbranchsi4_insn >>>> > in this case. This leads to ICE due to SET_SRC(label_insn) in subsequent >>>> > code. In general we should skip all such improper insns. This is the purpose >>>> > of attached small patch. >>>> > >>>> > Some failures in recent gcc regression test on thumb1 target are caused by >>>> > this reason. So with this patch, all of them passed and no new failures. Is >>>> > it ok to trunk? >>>> > >>>> > BR, >>>> > Terry >>>> > >>>> > 2014-06-10 Terry Guo <terry.guo@arm.com> >>>> > >>>> > * config/arm/arm.c (thumb1_reorg): Move to next basic block if the >>>> head >>>> > of current basic block isn't a proper insn. >>>> > >>>> >>>> I think you should just test that "insn != BB_HEAD (bb)". The loop >>>> immediately above this deals with the !NON-DEBUG insns, so the logic is >>>> confusing the way you've written it. >>>> >>>> R. >>>> >>> >>> Thanks for comments. The patch is updated and tested. No more ICE. Is this one OK? >>> >>> BR, >>> Terry >> >> Hi, >> The bug is also reported for 4.9 branch at >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61712 >> As far as I checked, the ICE is also caused by label instruction as in >> this message thread. >> Since the fix is on trunk more than two weeks, could we back port it >> to 4.9 branch? > > Yes, since there have been no other issues reported. Can either of you > also update PR61544 with the fields for Known to Work and Known to > fail setting a target milestone of GCC 4.9.1 rather than leaving these > fields empty. Done, I will back port it to 4.9 branch after 24 hours window. Hopefully we can catch with 4.9.1 release. Thanks, bin
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 85d2114..463707e 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -16946,7 +16946,8 @@ thumb1_reorg (void) insn = PREV_INSN (insn); /* Find the last cbranchsi4_insn in basic block BB. */ - if (INSN_CODE (insn) != CODE_FOR_cbranchsi4_insn) + if (insn == BB_HEAD (bb) + || INSN_CODE (insn) != CODE_FOR_cbranchsi4_insn) continue; /* Get the register with which we are comparing. */