| Message ID | 20140919193604.GA48001@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On 09/19/14 13:36, Ilya Enkovich wrote: > Hi, > > During my work on enabling pseudo PIC register I've found that cfg cleaunp may remove lables with LABEL_PRESERVE_P set to 1. In my case I generated SET_RIP during expand pass and cfg cleanup removed label it used as an operand. Below is a patch that fixes it. It is not actually required for our latest PIC related patch but still seems to make sense. > > Bootstrapped and tested on linux-x86_64. > > Thanks, > Ilya > -- > 2014-09-19 Ilya Enkovich <ilya.enkovich@intel.com> > > * cfgcleanup.c (try_optimize_cfg): Do not remove label > with LABEL_PRESERVE_P flag set. OK. Please install. Note for those not following the x86 32 bit PIC register discussion, I asked Ilya to submit this separately. It was something an earlier version of his patch triggered and it stood out as something that ought to be fixed regardless of the final form of the PIC register changes that are in progress. Thanks, Jeff
On Fri, Sep 19, 2014 at 10:03 PM, Jeff Law <law@redhat.com> wrote: > On 09/19/14 13:36, Ilya Enkovich wrote: >> >> Hi, >> >> During my work on enabling pseudo PIC register I've found that cfg cleaunp >> may remove lables with LABEL_PRESERVE_P set to 1. In my case I generated >> SET_RIP during expand pass and cfg cleanup removed label it used as an >> operand. Below is a patch that fixes it. It is not actually required for >> our latest PIC related patch but still seems to make sense. >> >> Bootstrapped and tested on linux-x86_64. >> >> Thanks, >> Ilya >> -- >> 2014-09-19 Ilya Enkovich <ilya.enkovich@intel.com> >> >> * cfgcleanup.c (try_optimize_cfg): Do not remove label >> with LABEL_PRESERVE_P flag set. > > OK. Please install. > > Note for those not following the x86 32 bit PIC register discussion, I asked > Ilya to submit this separately. It was something an earlier version of his > patch triggered and it stood out as something that ought to be fixed > regardless of the final form of the PIC register changes that are in > progress. Jeff, Are you sure this patch is necessary, and is not just papering over another problem? In the past, all cases I've seen where labels were removed inadvertently were caused by incorrect reference counting or missing REG_LABEL_* notes. Did the label use count drop to zero? Is there a REG_LABEL_TARGET note for the label operand? Ciao! Steven
On 09/23/14 10:01, Steven Bosscher wrote: > On Fri, Sep 19, 2014 at 10:03 PM, Jeff Law <law@redhat.com> wrote: >> On 09/19/14 13:36, Ilya Enkovich wrote: >>> >>> Hi, >>> >>> During my work on enabling pseudo PIC register I've found that cfg cleaunp >>> may remove lables with LABEL_PRESERVE_P set to 1. In my case I generated >>> SET_RIP during expand pass and cfg cleanup removed label it used as an >>> operand. Below is a patch that fixes it. It is not actually required for >>> our latest PIC related patch but still seems to make sense. >>> >>> Bootstrapped and tested on linux-x86_64. >>> >>> Thanks, >>> Ilya >>> -- >>> 2014-09-19 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> * cfgcleanup.c (try_optimize_cfg): Do not remove label >>> with LABEL_PRESERVE_P flag set. >> >> OK. Please install. >> >> Note for those not following the x86 32 bit PIC register discussion, I asked >> Ilya to submit this separately. It was something an earlier version of his >> patch triggered and it stood out as something that ought to be fixed >> regardless of the final form of the PIC register changes that are in >> progress. > > Jeff, > > Are you sure this patch is necessary, and is not just papering over > another problem? In the past, all cases I've seen where labels were > removed inadvertently were caused by incorrect reference counting or > missing REG_LABEL_* notes. > > Did the label use count drop to zero? Is there a REG_LABEL_TARGET note > for the label operand? The way it was described to me is, yes, the label count dropped to zero. In simplest terms, it was a single use label that was marked with LABEL_PRESERVE_P. The combiner removed the last reference, then cfgcleanup came along and *boom*. It was with some ongoing development work that's going in a slight different direction, so we don't have a testcase to include. jeff
2014-09-23 20:06 GMT+04:00 Jeff Law <law@redhat.com>: > On 09/23/14 10:01, Steven Bosscher wrote: >> >> On Fri, Sep 19, 2014 at 10:03 PM, Jeff Law <law@redhat.com> wrote: >>> >>> On 09/19/14 13:36, Ilya Enkovich wrote: >>>> >>>> >>>> Hi, >>>> >>>> During my work on enabling pseudo PIC register I've found that cfg >>>> cleaunp >>>> may remove lables with LABEL_PRESERVE_P set to 1. In my case I >>>> generated >>>> SET_RIP during expand pass and cfg cleanup removed label it used as an >>>> operand. Below is a patch that fixes it. It is not actually required >>>> for >>>> our latest PIC related patch but still seems to make sense. >>>> >>>> Bootstrapped and tested on linux-x86_64. >>>> >>>> Thanks, >>>> Ilya >>>> -- >>>> 2014-09-19 Ilya Enkovich <ilya.enkovich@intel.com> >>>> >>>> * cfgcleanup.c (try_optimize_cfg): Do not remove label >>>> with LABEL_PRESERVE_P flag set. >>> >>> >>> OK. Please install. >>> >>> Note for those not following the x86 32 bit PIC register discussion, I >>> asked >>> Ilya to submit this separately. It was something an earlier version of >>> his >>> patch triggered and it stood out as something that ought to be fixed >>> regardless of the final form of the PIC register changes that are in >>> progress. >> >> >> Jeff, >> >> Are you sure this patch is necessary, and is not just papering over >> another problem? In the past, all cases I've seen where labels were >> removed inadvertently were caused by incorrect reference counting or >> missing REG_LABEL_* notes. Description of LABEL_PRESERVE_P says label that should always be considered to be needed. That means even if we do not have any usages we shouldn't remove it. Why can't we add some additional usages later? >> >> Did the label use count drop to zero? Is there a REG_LABEL_TARGET note >> for the label operand? In the current code of ix86_expand_prologue I don't see any notes generation for set_rip_rex64 instruction which actually uses label. But IMO this is another potential issue and we still shouldn't remove labels with LABEL_PRESERVE_P. > > The way it was described to me is, yes, the label count dropped to zero. In > simplest terms, it was a single use label that was marked with > LABEL_PRESERVE_P. The combiner removed the last reference, then cfgcleanup > came along and *boom*. > There was also another case in 64bit target with large code model where I had combiner unrelated problem with removed label used by still existing set_rip_rex64. Ilya > It was with some ongoing development work that's going in a slight different > direction, so we don't have a testcase to include. > > jeff
On Wed, Sep 24, 2014 at 8:41 AM, Ilya Enkovich wrote: > 2014-09-23 20:06 GMT+04:00 Jeff Law: >> On 09/23/14 10:01, Steven Bosscher wrote: >>> Are you sure this patch is necessary, and is not just papering over >>> another problem? In the past, all cases I've seen where labels were >>> removed inadvertently were caused by incorrect reference counting or >>> missing REG_LABEL_* notes. > > Description of LABEL_PRESERVE_P says label that should always be > considered to be needed. It's more specific than that, really: @item LABEL_PRESERVE_P (@var{x}) In a @code{code_label} or @code{note}, indicates that the label is referenced by code or data not visible to the RTL of a given function. The "not visible" part is important. If there are visible references to a label, then they should never be removed (obviously) and that should work through LABEL_NUSES. Unfortunately we are not very good at keeping LABEL_NUSES up-to-date (this is why all the rebuild_jump_labels() are still required). What appears to be the case here, is that you have a label between two basic blocks B1 and B2, and the label acts as a control flow barrier: B1 and B2 cannot be merged. Then this should be expressed in the CFG. Otherwise: What else prevents the merge_blocks CFG hooks from deleting the label? > That means even if we do not have any usages > we shouldn't remove it. Sorry, no. Even a LABEL_PRESERVE_P label can be deleted: It will be replaced by a NOTE_INSN_DELETED_LABEL. See cfgrtl.c:delete_insn(). If you really want to prevent a label from being deleted, then LABEL_PRESERVE_P is not a sufficient condition. > Why can't we add some additional usages > later? If you add the usages later, then you're lying to the compiler ;-) >>> >>> Did the label use count drop to zero? Is there a REG_LABEL_TARGET note >>> for the label operand? > > In the current code of ix86_expand_prologue I don't see any notes > generation for set_rip_rex64 instruction which actually uses label. > But IMO this is another potential issue and we still shouldn't remove > labels with LABEL_PRESERVE_P. Notes are generated in jump.c:rebuild_jump_labels. They are automatically added when a label is not Ciao! Steven
2014-09-24 13:30 GMT+04:00 Steven Bosscher <stevenb.gcc@gmail.com>: > On Wed, Sep 24, 2014 at 8:41 AM, Ilya Enkovich wrote: >> 2014-09-23 20:06 GMT+04:00 Jeff Law: >>> On 09/23/14 10:01, Steven Bosscher wrote: >>>> Are you sure this patch is necessary, and is not just papering over >>>> another problem? In the past, all cases I've seen where labels were >>>> removed inadvertently were caused by incorrect reference counting or >>>> missing REG_LABEL_* notes. >> >> Description of LABEL_PRESERVE_P says label that should always be >> considered to be needed. > > It's more specific than that, really: > > @item LABEL_PRESERVE_P (@var{x}) > In a @code{code_label} or @code{note}, indicates that the label is referenced by > code or data not visible to the RTL of a given function. I read another description: /* 1 if RTX is a code_label that should always be considered to be needed. */ #define LABEL_PRESERVE_P(RTX) \ (RTL_FLAG_CHECK2 ("LABEL_PRESERVE_P", (RTX), CODE_LABEL, NOTE)->in_struct) > > > The "not visible" part is important. If there are visible references > to a label, then they should never be removed (obviously) and that > should work through LABEL_NUSES. Unfortunately we are not very good at > keeping LABEL_NUSES up-to-date (this is why all the > rebuild_jump_labels() are still required). Does rebuild handle all kinds of instructions including those which use UNSPEC? > > What appears to be the case here, is that you have a label between two > basic blocks B1 and B2, and the label acts as a control flow barrier: > B1 and B2 cannot be merged. Then this should be expressed in the CFG. > Otherwise: What else prevents the merge_blocks CFG hooks from deleting > the label? Label acts as a barrier here but it is a side effect. I don't care about block merging. I just don't want label with usages to be removed. > > > >> That means even if we do not have any usages >> we shouldn't remove it. > > Sorry, no. > Even a LABEL_PRESERVE_P label can be deleted: It will be replaced by a > NOTE_INSN_DELETED_LABEL. See cfgrtl.c:delete_insn(). According to description you quoted label marked by LABEL_PRESERVE_P is used by some code or data. Let this use be not visible to the RTL of a given function. It is still used, right? How can you remove it? Ilya > > If you really want to prevent a label from being deleted, then > LABEL_PRESERVE_P is not a sufficient condition. > > >> Why can't we add some additional usages >> later? > > If you add the usages later, then you're lying to the compiler ;-) > > >>>> >>>> Did the label use count drop to zero? Is there a REG_LABEL_TARGET note >>>> for the label operand? >> >> In the current code of ix86_expand_prologue I don't see any notes >> generation for set_rip_rex64 instruction which actually uses label. >> But IMO this is another potential issue and we still shouldn't remove >> labels with LABEL_PRESERVE_P. > > Notes are generated in jump.c:rebuild_jump_labels. They are > automatically added when a label is not > > Ciao! > Steven
On Wed, Sep 24, 2014 at 11:57 AM, Ilya Enkovich wrote: > 2014-09-24 13:30 GMT+04:00 Steven Bosscher : >>> Description of LABEL_PRESERVE_P says label that should always be >>> considered to be needed. >> >> It's more specific than that, really: >> >> @item LABEL_PRESERVE_P (@var{x}) >> In a @code{code_label} or @code{note}, indicates that the label is referenced by >> code or data not visible to the RTL of a given function. > > I read another description: > /* 1 if RTX is a code_label that should always be considered to be needed. */ > #define LABEL_PRESERVE_P(RTX) \ > (RTL_FLAG_CHECK2 ("LABEL_PRESERVE_P", (RTX), CODE_LABEL, NOTE)->in_struct) Yes, from rtl.h. I'd recommend to always read the descriptions in doc/ (in this case doc/rtl.texi). The "documentation" in the header files is often not very comprehensive. >> The "not visible" part is important. If there are visible references >> to a label, then they should never be removed (obviously) and that >> should work through LABEL_NUSES. Unfortunately we are not very good at >> keeping LABEL_NUSES up-to-date (this is why all the >> rebuild_jump_labels() are still required). > > Does rebuild handle all kinds of instructions including those which use UNSPEC? Yes. Patterns are walked (deep) and REG_LABEL notes are added for all labels encountered that are not already the JUMP_LABEL of INSN. If the label is reachable from XEXP(UNSPEC, 0) -- the 'E' operand -- then that label is visible. >> What appears to be the case here, is that you have a label between two >> basic blocks B1 and B2, and the label acts as a control flow barrier: >> B1 and B2 cannot be merged. Then this should be expressed in the CFG. >> Otherwise: What else prevents the merge_blocks CFG hooks from deleting >> the label? > > Label acts as a barrier here but it is a side effect. I don't care > about block merging. I just don't want label with usages to be > removed. Understood. Only, LABEL_PRESERVE_P is not the right means to achieve that. So let's get back to basics and see what the usages look like. AFAIU now, you emit the code label early, and add the references much later (in machine reorg?). Does your UNSPEC have the code_label as an operand? If so, what breaks if cfgcleanup removes the label? Is the insn no longer recognized? Or does the label not end up in the assembly output? Or ...? I can try to help figure out what breaks if you have a test case. FWIW, the LABEL_PRESERVE_P uses in config/i386/i386.c look suspect. It probably only works because those labels are added late, and the code paths that use (x86_64 large PIC code model) are not tested all that well... >>> That means even if we do not have any usages >>> we shouldn't remove it. >> >> Sorry, no. >> Even a LABEL_PRESERVE_P label can be deleted: It will be replaced by a >> NOTE_INSN_DELETED_LABEL. See cfgrtl.c:delete_insn(). > > According to description you quoted label marked by LABEL_PRESERVE_P > is used by some code or data. Let this use be not visible to the RTL > of a given function. It is still used, right? How can you remove it? The code_label rtx is removed, but the label itself is still output to the object file. The label number is retained in the CODE_LABEL_NUMBER of the NOTE_INSN_DELETED_LABEL. Look for how NOTE_INSN_DELETED_LABEL is handled in final.c. It's a hack IMHO, but that's how it has been since day 0 (see https://gcc.gnu.org/r104). Ciao! Steven
2014-09-24 14:35 GMT+04:00 Steven Bosscher <stevenb.gcc@gmail.com>: > On Wed, Sep 24, 2014 at 11:57 AM, Ilya Enkovich wrote: >> 2014-09-24 13:30 GMT+04:00 Steven Bosscher : >>>> Description of LABEL_PRESERVE_P says label that should always be >>>> considered to be needed. >>> >>> It's more specific than that, really: >>> >>> @item LABEL_PRESERVE_P (@var{x}) >>> In a @code{code_label} or @code{note}, indicates that the label is referenced by >>> code or data not visible to the RTL of a given function. >> >> I read another description: >> /* 1 if RTX is a code_label that should always be considered to be needed. */ >> #define LABEL_PRESERVE_P(RTX) \ >> (RTL_FLAG_CHECK2 ("LABEL_PRESERVE_P", (RTX), CODE_LABEL, NOTE)->in_struct) > > Yes, from rtl.h. > > I'd recommend to always read the descriptions in doc/ (in this case > doc/rtl.texi). The "documentation" in the header files is often not > very comprehensive. > > >>> The "not visible" part is important. If there are visible references >>> to a label, then they should never be removed (obviously) and that >>> should work through LABEL_NUSES. Unfortunately we are not very good at >>> keeping LABEL_NUSES up-to-date (this is why all the >>> rebuild_jump_labels() are still required). >> >> Does rebuild handle all kinds of instructions including those which use UNSPEC? > > Yes. Patterns are walked (deep) and REG_LABEL notes are added for all > labels encountered that are not already the JUMP_LABEL of INSN. If the > label is reachable from XEXP(UNSPEC, 0) -- the 'E' operand -- then > that label is visible. > > >>> What appears to be the case here, is that you have a label between two >>> basic blocks B1 and B2, and the label acts as a control flow barrier: >>> B1 and B2 cannot be merged. Then this should be expressed in the CFG. >>> Otherwise: What else prevents the merge_blocks CFG hooks from deleting >>> the label? >> >> Label acts as a barrier here but it is a side effect. I don't care >> about block merging. I just don't want label with usages to be >> removed. > > Understood. Only, LABEL_PRESERVE_P is not the right means to achieve that. > > So let's get back to basics and see what the usages look like. AFAIU > now, you emit the code label early, and add the references much later > (in machine reorg?). Does your UNSPEC have the code_label as an > operand? If so, what breaks if cfgcleanup removes the label? Is the > insn no longer recognized? Or does the label not end up in the > assembly output? Or ...? I can try to help figure out what breaks if > you have a test case. > > FWIW, the LABEL_PRESERVE_P uses in config/i386/i386.c look suspect. It > probably only works because those labels are added late, and the code > paths that use (x86_64 large PIC code model) are not tested all that > well... I didn't generate references separately from label. Now I found an old patch and a test where this problem appeared. In this patch I moved set_rip generation currently performed in ix86_expand_prologue into expand pass. And I got following code in expand dump for testsuite/gcc.target/i386/pr55154.c test: (note 7 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK) (note/s 2 7 3 2 "" NOTE_INSN_DELETED_LABEL 2) (insn 3 2 4 2 (set (reg:DI 85) (unspec:DI [ (label_ref [2 deleted]) ] UNSPEC_SET_RIP)) /export/users/ienkovic/issues/4161/gcc/gcc/testsuite/gcc.target/i386/pr55154.c:9 -1 (insn_list:REG_LABEL_OPERAND 2 (nil))) There is a REG_LABEL_OPERAND generated but label is still removed. Ilya > > >>>> That means even if we do not have any usages >>>> we shouldn't remove it. >>> >>> Sorry, no. >>> Even a LABEL_PRESERVE_P label can be deleted: It will be replaced by a >>> NOTE_INSN_DELETED_LABEL. See cfgrtl.c:delete_insn(). >> >> According to description you quoted label marked by LABEL_PRESERVE_P >> is used by some code or data. Let this use be not visible to the RTL >> of a given function. It is still used, right? How can you remove it? > > The code_label rtx is removed, but the label itself is still output to > the object file. The label number is retained in the CODE_LABEL_NUMBER > of the NOTE_INSN_DELETED_LABEL. Look for how NOTE_INSN_DELETED_LABEL > is handled in final.c. It's a hack IMHO, but that's how it has been > since day 0 (see https://gcc.gnu.org/r104). > > Ciao! > Steven
On Wed, Sep 24, 2014 at 2:30 PM, Ilya Enkovich wrote: > I didn't generate references separately from label. Now I found an > old patch and a test where this problem appeared. In this patch I > moved set_rip generation currently performed in ix86_expand_prologue > into expand pass. And I got following code in expand dump for > testsuite/gcc.target/i386/pr55154.c test: > > (note 7 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK) > (note/s 2 7 3 2 "" NOTE_INSN_DELETED_LABEL 2) > (insn 3 2 4 2 (set (reg:DI 85) > (unspec:DI [ > (label_ref [2 deleted]) > ] UNSPEC_SET_RIP)) > /export/users/ienkovic/issues/4161/gcc/gcc/testsuite/gcc.target/i386/pr55154.c:9 > -1 > (insn_list:REG_LABEL_OPERAND 2 (nil))) > > There is a REG_LABEL_OPERAND generated but label is still removed. Because it should be a REG_LABEL_TARGET? AFAUI this is a contol flow insn so I'd expect it to be a jump_insn (and the note will be a TARGET note). But it's not a PC-set insn and a jump target the compiler will interpret as an infinite loop (if the insns are really in the order as above) which is clearly not what you want. So if you emit it as a jump_insn I'm not sure what will happen... Is it necessary to emit the label into a basic block? Ciao! Steven
2014-09-24 16:47 GMT+04:00 Steven Bosscher <stevenb.gcc@gmail.com>: > On Wed, Sep 24, 2014 at 2:30 PM, Ilya Enkovich wrote: >> I didn't generate references separately from label. Now I found an >> old patch and a test where this problem appeared. In this patch I >> moved set_rip generation currently performed in ix86_expand_prologue >> into expand pass. And I got following code in expand dump for >> testsuite/gcc.target/i386/pr55154.c test: >> >> (note 7 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK) >> (note/s 2 7 3 2 "" NOTE_INSN_DELETED_LABEL 2) >> (insn 3 2 4 2 (set (reg:DI 85) >> (unspec:DI [ >> (label_ref [2 deleted]) >> ] UNSPEC_SET_RIP)) >> /export/users/ienkovic/issues/4161/gcc/gcc/testsuite/gcc.target/i386/pr55154.c:9 >> -1 >> (insn_list:REG_LABEL_OPERAND 2 (nil))) >> >> There is a REG_LABEL_OPERAND generated but label is still removed. > > Because it should be a REG_LABEL_TARGET? > > AFAUI this is a contol flow insn so I'd expect it to be a jump_insn > (and the note will be a TARGET note). But it's not a PC-set insn and a > jump target the compiler will interpret as an infinite loop (if the > insns are really in the order as above) which is clearly not what you > want. So if you emit it as a jump_insn I'm not sure what will > happen... > > Is it necessary to emit the label into a basic block? It is not a control flow instruction. It copies value of instruction pointer into a general purpose register. Therefore REG_LABEL_OPERAND seems to be correct. Ilya > > Ciao! > Steven
On 09/24/14 03:30, Steven Bosscher wrote: > > What appears to be the case here, is that you have a label between two > basic blocks B1 and B2, and the label acts as a control flow barrier: > B1 and B2 cannot be merged. Then this should be expressed in the CFG. > Otherwise: What else prevents the merge_blocks CFG hooks from deleting > the label? A part of me wonders why we have the label at all. Can't we just use dot-relative addressing. But yes, exposing the PIC setup label is problematical in various ways. We've gone a different direction in later patches, but the underlying issues are worth continuing to dig into. >> That means even if we do not have any usages >> we shouldn't remove it. > > Sorry, no. > Even a LABEL_PRESERVE_P label can be deleted: It will be replaced by a > NOTE_INSN_DELETED_LABEL. See cfgrtl.c:delete_insn(). > > If you really want to prevent a label from being deleted, then > LABEL_PRESERVE_P is not a sufficient condition. > > >> Why can't we add some additional usages >> later? > > If you add the usages later, then you're lying to the compiler ;-) Perhaps. The problem is while we can expose all the known uses early, reload/LRA via rematerialization can create new ones, even after all the old ones have gone away. It's a not terribly uncommon problem for reload/LRA to generate new symbolic references to objects when rematerializing certain constants. Those new references will expose new uses of the PIC register and consequently perhaps the PIC setup label. This puts us in an interesting position WRT LABEL_PRESERVE_P. Jeff
On 09/24/14 04:35, Steven Bosscher wrote: >> >> According to description you quoted label marked by LABEL_PRESERVE_P >> is used by some code or data. Let this use be not visible to the RTL >> of a given function. It is still used, right? How can you remove it? > > The code_label rtx is removed, but the label itself is still output to > the object file. The label number is retained in the CODE_LABEL_NUMBER > of the NOTE_INSN_DELETED_LABEL. Look for how NOTE_INSN_DELETED_LABEL > is handled in final.c. It's a hack IMHO, but that's how it has been > since day 0 (see https://gcc.gnu.org/r104). Right. We transform it into a NOTE_INSN_DELETED_LABEL, but still output it into the assembly code. There's one other intereseting aspect here -- IIRC, we allow NOTE_INSN_DELETED_LABELs to move around the insn stream. My recollection was we may move them from between two blocks to the start of a block to facilitate block merging. Basically the LABEL_PRESERVE_P ensures the label gets output in the assembly code. THe docs could possibly use some clarification. Jeff
2014-09-24 23:03 GMT+04:00 Jeff Law <law@redhat.com>: > On 09/24/14 04:35, Steven Bosscher wrote: >>> >>> >>> According to description you quoted label marked by LABEL_PRESERVE_P >>> is used by some code or data. Let this use be not visible to the RTL >>> of a given function. It is still used, right? How can you remove it? >> >> >> The code_label rtx is removed, but the label itself is still output to >> the object file. The label number is retained in the CODE_LABEL_NUMBER >> of the NOTE_INSN_DELETED_LABEL. Look for how NOTE_INSN_DELETED_LABEL >> is handled in final.c. It's a hack IMHO, but that's how it has been >> since day 0 (see https://gcc.gnu.org/r104). > > Right. We transform it into a NOTE_INSN_DELETED_LABEL, but still output it > into the assembly code. There's one other intereseting aspect here -- IIRC, > we allow NOTE_INSN_DELETED_LABELs to move around the insn stream. My > recollection was we may move them from between two blocks to the start of a > block to facilitate block merging. > > Basically the LABEL_PRESERVE_P ensures the label gets output in the assembly > code. THe docs could possibly use some clarification. > > Jeff The patch has been already installed. Should it be reverted for now then? Or probably test and install a version with LABEL_NUSES? It still doesn't looks correct that cfgcleanup may remove label with nonzero count of uses with following ICE in optimizers. Ilya
On 09/24/14 14:38, Ilya Enkovich wrote: > 2014-09-24 23:03 GMT+04:00 Jeff Law <law@redhat.com>: >> On 09/24/14 04:35, Steven Bosscher wrote: >>>> >>>> >>>> According to description you quoted label marked by LABEL_PRESERVE_P >>>> is used by some code or data. Let this use be not visible to the RTL >>>> of a given function. It is still used, right? How can you remove it? >>> >>> >>> The code_label rtx is removed, but the label itself is still output to >>> the object file. The label number is retained in the CODE_LABEL_NUMBER >>> of the NOTE_INSN_DELETED_LABEL. Look for how NOTE_INSN_DELETED_LABEL >>> is handled in final.c. It's a hack IMHO, but that's how it has been >>> since day 0 (see https://gcc.gnu.org/r104). >> >> Right. We transform it into a NOTE_INSN_DELETED_LABEL, but still output it >> into the assembly code. There's one other intereseting aspect here -- IIRC, >> we allow NOTE_INSN_DELETED_LABELs to move around the insn stream. My >> recollection was we may move them from between two blocks to the start of a >> block to facilitate block merging. >> >> Basically the LABEL_PRESERVE_P ensures the label gets output in the assembly >> code. THe docs could possibly use some clarification. >> >> Jeff > > The patch has been already installed. Should it be reverted for now > then? Or probably test and install a version with LABEL_NUSES? It > still doesn't looks correct that cfgcleanup may remove label with > nonzero count of uses with following ICE in optimizers. I'd leave it, but if Steven wants to remove and replace with a different test I won't object. Jeff
diff --git a/gcc/cfgcleanup.c b/gcc/cfgcleanup.c index a008168..9325ea0 100644 --- a/gcc/cfgcleanup.c +++ b/gcc/cfgcleanup.c @@ -2701,6 +2701,7 @@ try_optimize_cfg (int mode) && (single_pred_edge (b)->flags & EDGE_FALLTHRU) && !(single_pred_edge (b)->flags & EDGE_COMPLEX) && LABEL_P (BB_HEAD (b)) + && !LABEL_PRESERVE_P (BB_HEAD (b)) /* If the previous block ends with a branch to this block, we can't delete the label. Normally this is a condjump that is yet to be simplified, but