| Message ID | CAOvf_xxtWrrAm_z3s3-xGMqkPPA7SppL-woFG-4docMrmZ1xGw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Oct 17, 2014 at 06:16:41PM +0400, Evgeny Stupachenko wrote: > Hi, > > Some instructions (like one in PR63534) could have hidden use of PIC register. > Therefore we need to leave SET_GOT not deleted till reload completed. > The patch prevents SET_GOT from deleting while PIC register is pseudo. Just curious, do you emit the init_pic_reg unconditionally at the start of the function in -fpic mode? What does IRA do in that case, if it sees a dead setter of something that doesn't seem to be used at that point? Doesn't it penalize generated code, even if we don't end up with any PIC references during/after reload? Jakub
Yes, unconditionally. If pic_reg is unused, RA will allocate a hard register for it and treat it as free, DCE after reload will delete SET_GOT. On Fri, Oct 17, 2014 at 6:20 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Oct 17, 2014 at 06:16:41PM +0400, Evgeny Stupachenko wrote: >> Hi, >> >> Some instructions (like one in PR63534) could have hidden use of PIC register. >> Therefore we need to leave SET_GOT not deleted till reload completed. >> The patch prevents SET_GOT from deleting while PIC register is pseudo. > > Just curious, do you emit the init_pic_reg unconditionally at the start of > the function in -fpic mode? What does IRA do in that case, if it sees > a dead setter of something that doesn't seem to be used at that point? > Doesn't it penalize generated code, even if we don't end up with any PIC > references during/after reload? > > Jakub
On 10/17/14 08:16, Evgeny Stupachenko wrote: > Hi, > > Some instructions (like one in PR63534) could have hidden use of PIC register. > Therefore we need to leave SET_GOT not deleted till reload completed. > The patch prevents SET_GOT from deleting while PIC register is pseudo. > > Is it ok? > > ChangeLog: > > 2014-10-17 Evgeny Stupachenko <evstupac@gmail.com> > > PR target/63534 > * cse.c (delete_trivially_dead_insns): Consider PIC register is used > while it is pseudo. > * dse.c (deletable_insn_p): Likewise. > > diff --git a/gcc/cse.c b/gcc/cse.c > index be2f31b..062ba45 100644 > --- a/gcc/cse.c > +++ b/gcc/cse.c > @@ -6953,6 +6953,11 @@ delete_trivially_dead_insns (rtx_insn *insns, int nreg) > /* If no debug insns can be present, COUNTS is just an array > which counts how many times each pseudo is used. */ > } > + /* Pseudo PIC register should be considered as used due to possible > + new usages generated. */ > + if (pic_offset_table_rtx > + && REGNO (pic_offset_table_rtx) >= FIRST_PSEUDO_REGISTER) > + counts[REGNO (pic_offset_table_rtx)]++; > /* Go from the last insn to the first and delete insns that only set unused > registers or copy a register to itself. As we delete an insn, remove > usage counts for registers it uses. Shouldn't this also be guarded with !reload_completed? One reload is complete all the implicit references to the PIC register should be explicit and thus there's no need to treat the PIC register special. > diff --git a/gcc/dce.c b/gcc/dce.c > index 5b7d36e..a52a59c 100644 > --- a/gcc/dce.c > +++ b/gcc/dce.c > @@ -127,6 +127,10 @@ deletable_insn_p (rtx_insn *insn, bool fast, > bitmap arg_stores) > if (HARD_REGISTER_NUM_P (DF_REF_REGNO (def)) > && global_regs[DF_REF_REGNO (def)]) > return false; > + /* Initialization of pseudo PIC register should never be removed. */ > + else if (DF_REF_REG (def) == pic_offset_table_rtx > + && REGNO (pic_offset_table_rtx) >= FIRST_PSEUDO_REGISTER) > + return false; Similarly. jeff
On Wed, Oct 22, 2014 at 03:34:38PM -0600, Jeff Law wrote: > >--- a/gcc/cse.c > >+++ b/gcc/cse.c > >@@ -6953,6 +6953,11 @@ delete_trivially_dead_insns (rtx_insn *insns, int nreg) > > /* If no debug insns can be present, COUNTS is just an array > > which counts how many times each pseudo is used. */ > > } > >+ /* Pseudo PIC register should be considered as used due to possible > >+ new usages generated. */ > >+ if (pic_offset_table_rtx > >+ && REGNO (pic_offset_table_rtx) >= FIRST_PSEUDO_REGISTER) > >+ counts[REGNO (pic_offset_table_rtx)]++; > > /* Go from the last insn to the first and delete insns that only set unused > > registers or copy a register to itself. As we delete an insn, remove > > usage counts for registers it uses. > Shouldn't this also be guarded with !reload_completed? One reload is > complete all the implicit references to the PIC register should be explicit > and thus there's no need to treat the PIC register special. Supposedly this one yes. > >diff --git a/gcc/dce.c b/gcc/dce.c > >index 5b7d36e..a52a59c 100644 > >--- a/gcc/dce.c > >+++ b/gcc/dce.c > >@@ -127,6 +127,10 @@ deletable_insn_p (rtx_insn *insn, bool fast, > >bitmap arg_stores) > > if (HARD_REGISTER_NUM_P (DF_REF_REGNO (def)) > > && global_regs[DF_REF_REGNO (def)]) > > return false; > >+ /* Initialization of pseudo PIC register should never be removed. */ > >+ else if (DF_REF_REG (def) == pic_offset_table_rtx > >+ && REGNO (pic_offset_table_rtx) >= FIRST_PSEUDO_REGISTER) > >+ return false; > Similarly. But here, after reload completes, there shouldn't be pseudos in the IL, so the condition should not trigger anymore. Jakub
>But here, after reload completes, there shouldn't be pseudos in the IL, so >the condition should not trigger anymore. At that point we'll just exit the condition earlier with !reload_completed. On Thu, Oct 23, 2014 at 1:40 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Oct 22, 2014 at 03:34:38PM -0600, Jeff Law wrote: >> >--- a/gcc/cse.c >> >+++ b/gcc/cse.c >> >@@ -6953,6 +6953,11 @@ delete_trivially_dead_insns (rtx_insn *insns, int nreg) >> > /* If no debug insns can be present, COUNTS is just an array >> > which counts how many times each pseudo is used. */ >> > } >> >+ /* Pseudo PIC register should be considered as used due to possible >> >+ new usages generated. */ >> >+ if (pic_offset_table_rtx >> >+ && REGNO (pic_offset_table_rtx) >= FIRST_PSEUDO_REGISTER) >> >+ counts[REGNO (pic_offset_table_rtx)]++; >> > /* Go from the last insn to the first and delete insns that only set unused >> > registers or copy a register to itself. As we delete an insn, remove >> > usage counts for registers it uses. >> Shouldn't this also be guarded with !reload_completed? One reload is >> complete all the implicit references to the PIC register should be explicit >> and thus there's no need to treat the PIC register special. > > Supposedly this one yes. > >> >diff --git a/gcc/dce.c b/gcc/dce.c >> >index 5b7d36e..a52a59c 100644 >> >--- a/gcc/dce.c >> >+++ b/gcc/dce.c >> >@@ -127,6 +127,10 @@ deletable_insn_p (rtx_insn *insn, bool fast, >> >bitmap arg_stores) >> > if (HARD_REGISTER_NUM_P (DF_REF_REGNO (def)) >> > && global_regs[DF_REF_REGNO (def)]) >> > return false; >> >+ /* Initialization of pseudo PIC register should never be removed. */ >> >+ else if (DF_REF_REG (def) == pic_offset_table_rtx >> >+ && REGNO (pic_offset_table_rtx) >= FIRST_PSEUDO_REGISTER) >> >+ return false; >> Similarly. > > But here, after reload completes, there shouldn't be pseudos in the IL, so > the condition should not trigger anymore. > > Jakub
On 10/22/14 15:40, Jakub Jelinek wrote: > >>> diff --git a/gcc/dce.c b/gcc/dce.c >>> index 5b7d36e..a52a59c 100644 >>> --- a/gcc/dce.c >>> +++ b/gcc/dce.c >>> @@ -127,6 +127,10 @@ deletable_insn_p (rtx_insn *insn, bool fast, >>> bitmap arg_stores) >>> if (HARD_REGISTER_NUM_P (DF_REF_REGNO (def)) >>> && global_regs[DF_REF_REGNO (def)]) >>> return false; >>> + /* Initialization of pseudo PIC register should never be removed. */ >>> + else if (DF_REF_REG (def) == pic_offset_table_rtx >>> + && REGNO (pic_offset_table_rtx) >= FIRST_PSEUDO_REGISTER) >>> + return false; >> Similarly. > > But here, after reload completes, there shouldn't be pseudos in the IL, so > the condition should not trigger anymore. Agreed. So just the first one needs to check for !reload_completed. With that change, and a bootstrap/regression run the patch is OK for the trunk. Jeff
diff --git a/gcc/cse.c b/gcc/cse.c index be2f31b..062ba45 100644 --- a/gcc/cse.c +++ b/gcc/cse.c @@ -6953,6 +6953,11 @@ delete_trivially_dead_insns (rtx_insn *insns, int nreg) /* If no debug insns can be present, COUNTS is just an array which counts how many times each pseudo is used. */ } + /* Pseudo PIC register should be considered as used due to possible + new usages generated. */ + if (pic_offset_table_rtx + && REGNO (pic_offset_table_rtx) >= FIRST_PSEUDO_REGISTER) + counts[REGNO (pic_offset_table_rtx)]++; /* Go from the last insn to the first and delete insns that only set unused registers or copy a register to itself. As we delete an insn, remove usage counts for registers it uses. diff --git a/gcc/dce.c b/gcc/dce.c index 5b7d36e..a52a59c 100644 --- a/gcc/dce.c +++ b/gcc/dce.c @@ -127,6 +127,10 @@ deletable_insn_p (rtx_insn *insn, bool fast, bitmap arg_stores) if (HARD_REGISTER_NUM_P (DF_REF_REGNO (def)) && global_regs[DF_REF_REGNO (def)]) return false; + /* Initialization of pseudo PIC register should never be removed. */ + else if (DF_REF_REG (def) == pic_offset_table_rtx + && REGNO (pic_offset_table_rtx) >= FIRST_PSEUDO_REGISTER) + return false; body = PATTERN (insn);
Hi, Some instructions (like one in PR63534) could have hidden use of PIC register. Therefore we need to leave SET_GOT not deleted till reload completed. The patch prevents SET_GOT from deleting while PIC register is pseudo. Is it ok? ChangeLog: 2014-10-17 Evgeny Stupachenko <evstupac@gmail.com> PR target/63534 * cse.c (delete_trivially_dead_insns): Consider PIC register is used while it is pseudo. * dse.c (deletable_insn_p): Likewise. switch (GET_CODE (body))