| Message ID | d86b5611543c21fa40ba8b5a368179c00df930c4.1465347472.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On 06/08/2016 03:47 AM, Segher Boessenkool wrote: > + /* regrename creates wrong code for exception handling, if used together > + with separate shrink-wrapping. Disable for now, until we have > + figured out what exactly is going on. */ That needs to be figured out now or it'll be there forever. Bernd
On 06/08/2016 03:18 AM, Bernd Schmidt wrote: > On 06/08/2016 03:47 AM, Segher Boessenkool wrote: >> + /* regrename creates wrong code for exception handling, if used >> together >> + with separate shrink-wrapping. Disable for now, until we have >> + figured out what exactly is going on. */ > > That needs to be figured out now or it'll be there forever. I suspect it's related to liveness computations getting out-of-wack with separate shrink wrapping. If that's the case, then the question in my mind is how painful is this going to be to fix in the df scanning code. jeff
On Fri, Sep 09, 2016 at 12:31:31PM -0600, Jeff Law wrote: > On 06/08/2016 03:18 AM, Bernd Schmidt wrote: > >On 06/08/2016 03:47 AM, Segher Boessenkool wrote: > >>+ /* regrename creates wrong code for exception handling, if used > >>together > >>+ with separate shrink-wrapping. Disable for now, until we have > >>+ figured out what exactly is going on. */ > > > >That needs to be figured out now or it'll be there forever. > I suspect it's related to liveness computations getting out-of-wack with > separate shrink wrapping. If that's the case, then the question in my > mind is how painful is this going to be to fix in the df scanning code. I haven't been able to pin-point the failure. It happens for just a few huge testcases. So I am hoping someone who understands regrename will figure it out. Segher
On 09/09/2016 02:41 PM, Segher Boessenkool wrote: > On Fri, Sep 09, 2016 at 12:31:31PM -0600, Jeff Law wrote: >> On 06/08/2016 03:18 AM, Bernd Schmidt wrote: >>> On 06/08/2016 03:47 AM, Segher Boessenkool wrote: >>>> + /* regrename creates wrong code for exception handling, if used >>>> together >>>> + with separate shrink-wrapping. Disable for now, until we have >>>> + figured out what exactly is going on. */ >>> >>> That needs to be figured out now or it'll be there forever. >> I suspect it's related to liveness computations getting out-of-wack with >> separate shrink wrapping. If that's the case, then the question in my >> mind is how painful is this going to be to fix in the df scanning code. > > I haven't been able to pin-point the failure. It happens for just a few > huge testcases. So I am hoping someone who understands regrename will > figure it out. I think that's likely going to fall onto you :-) We don't generally allow passes to just get disabled because some other pass causes them to generate the wrong code. Though I'm curious how you triggered this -- regrename isn't enabled by default (except for that brief window earlier this year...). Jeff
On Fri, Sep 09, 2016 at 05:01:19PM -0600, Jeff Law wrote: > On 09/09/2016 02:41 PM, Segher Boessenkool wrote: > >On Fri, Sep 09, 2016 at 12:31:31PM -0600, Jeff Law wrote: > >>On 06/08/2016 03:18 AM, Bernd Schmidt wrote: > >>>On 06/08/2016 03:47 AM, Segher Boessenkool wrote: > >>>>+ /* regrename creates wrong code for exception handling, if used > >>>>together > >>>>+ with separate shrink-wrapping. Disable for now, until we have > >>>>+ figured out what exactly is going on. */ > >>> > >>>That needs to be figured out now or it'll be there forever. > >>I suspect it's related to liveness computations getting out-of-wack with > >>separate shrink wrapping. If that's the case, then the question in my > >>mind is how painful is this going to be to fix in the df scanning code. > > > >I haven't been able to pin-point the failure. It happens for just a few > >huge testcases. So I am hoping someone who understands regrename will > >figure it out. > I think that's likely going to fall onto you :-) We don't generally > allow passes to just get disabled because some other pass causes them to > generate the wrong code. We can instead declare that anyone enabling regrename is on his own? I like that plan better. I can also make the compiler error out if you try to have both separate shrink-wrapping and regrename on at the same time. There are no happy answers :-( > Though I'm curious how you triggered this -- regrename isn't enabled by > default (except for that brief window earlier this year...). Yes, these patches are that old now already. I also enabled regrename by default for quite a while, for testing purposes, since at the time it looked like regrename would be on by default for most architectures. Separate shrink-wrapping is supposed to be useful for all ports, not just for Power. Segher
On 09/10/2016 12:40 AM, Segher Boessenkool wrote: > On Fri, Sep 09, 2016 at 05:01:19PM -0600, Jeff Law wrote: >> On 09/09/2016 02:41 PM, Segher Boessenkool wrote: >>> On Fri, Sep 09, 2016 at 12:31:31PM -0600, Jeff Law wrote: >>>> On 06/08/2016 03:18 AM, Bernd Schmidt wrote: >>>>> On 06/08/2016 03:47 AM, Segher Boessenkool wrote: >>>>>> + /* regrename creates wrong code for exception handling, if used >>>>>> together >>>>>> + with separate shrink-wrapping. Disable for now, until we have >>>>>> + figured out what exactly is going on. */ >>>>> >>>>> That needs to be figured out now or it'll be there forever. >>>> I suspect it's related to liveness computations getting out-of-wack with >>>> separate shrink wrapping. If that's the case, then the question in my >>>> mind is how painful is this going to be to fix in the df scanning code. >>> >>> I haven't been able to pin-point the failure. It happens for just a few >>> huge testcases. So I am hoping someone who understands regrename will >>> figure it out. >> I think that's likely going to fall onto you :-) We don't generally >> allow passes to just get disabled because some other pass causes them to >> generate the wrong code. > > We can instead declare that anyone enabling regrename is on his own? > I like that plan better. No. I won't ack that. It seems totally backwards. > > I can also make the compiler error out if you try to have both separate > shrink-wrapping and regrename on at the same time. > > There are no happy answers :-( The answer is to debug the problem, and yes it may be painful. I'd probably start by fixing the dataflow issues and see if that fixes the regrename thing as a side effect. > >> Though I'm curious how you triggered this -- regrename isn't enabled by >> default (except for that brief window earlier this year...). > > Yes, these patches are that old now already. I also enabled regrename > by default for quite a while, for testing purposes, since at the time > it looked like regrename would be on by default for most architectures. > Separate shrink-wrapping is supposed to be useful for all ports, not > just for Power. I think enabling regrename by default is the right thing to do long term. jeff
On 09/12/2016 10:54 AM, David Edelsohn wrote: > On Sep 12, 2016 17:33, "Jeff Law" <law@redhat.com > <mailto:law@redhat.com>> wrote: >> >> On 09/10/2016 12:40 AM, Segher Boessenkool wrote: >>> >>> On Fri, Sep 09, 2016 at 05:01:19PM -0600, Jeff Law wrote: >>>> >>>> On 09/09/2016 02:41 PM, Segher Boessenkool wrote: >>>>> >>>>> On Fri, Sep 09, 2016 at 12:31:31PM -0600, Jeff Law wrote: >>>>>> >>>>>> On 06/08/2016 03:18 AM, Bernd Schmidt wrote: >>>>>>> >>>>>>> On 06/08/2016 03:47 AM, Segher Boessenkool wrote: >>>>>>>> >>>>>>>> + /* regrename creates wrong code for exception handling, > if used >>>>>>>> together >>>>>>>> + with separate shrink-wrapping. Disable for now, until > we have >>>>>>>> + figured out what exactly is going on. */ >>>>>>> >>>>>>> >>>>>>> That needs to be figured out now or it'll be there forever. >>>>>> >>>>>> I suspect it's related to liveness computations getting > out-of-wack with >>>>>> separate shrink wrapping. If that's the case, then the question in my >>>>>> mind is how painful is this going to be to fix in the df scanning > code. >>>>> >>>>> >>>>> I haven't been able to pin-point the failure. It happens for just > a few >>>>> huge testcases. So I am hoping someone who understands regrename will >>>>> figure it out. >>>> >>>> I think that's likely going to fall onto you :-) We don't generally >>>> allow passes to just get disabled because some other pass causes them to >>>> generate the wrong code. >>> >>> >>> We can instead declare that anyone enabling regrename is on his own? >>> I like that plan better. >> >> No. I won't ack that. It seems totally backwards. >> >> >>> >>> I can also make the compiler error out if you try to have both separate >>> shrink-wrapping and regrename on at the same time. >>> >>> There are no happy answers :-( >> >> The answer is to debug the problem, and yes it may be painful. I'd > probably start by fixing the dataflow issues and see if that fixes the > regrename thing as a side effect. >> >> >>> >>>> Though I'm curious how you triggered this -- regrename isn't enabled by >>>> default (except for that brief window earlier this year...). >>> >>> >>> Yes, these patches are that old now already. I also enabled regrename >>> by default for quite a while, for testing purposes, since at the time >>> it looked like regrename would be on by default for most architectures. >>> Separate shrink-wrapping is supposed to be useful for all ports, not >>> just for Power. >> >> I think enabling regrename by default is the right thing to do long term. > > Jeff, > > The current regrename pass is horrible. Badly designed, badly written > and ineffective. You are the only person who likes it. ?!? I don't like or dislike it. I think it solves a well known problem for processors with particular pipeline characteristics. No more, no less. > > There are plenty of gcc passes that don't work well together. If segher > can work around it easily, okay. This series of patches should not be > held up because of a bug in a badly written pass that is not enabled by > default and only found by segher being extra diligent. It's not a question of not working well together. It's a case where the two in combination generate incorrect code and we don't know if it's a regrename issue or an issue with Segher's code. I have some suspicions here, but until Segher does the analysis, I can't ack the patch. jeff
On Mon, Sep 12, 2016 at 10:33:22AM -0600, Jeff Law wrote: > The answer is to debug the problem, and yes it may be painful. Yes, the three weeks pretty much full-time I spent on that already attest to that. > I'd > probably start by fixing the dataflow issues and see if that fixes the > regrename thing as a side effect. Have you seen https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00091.html ? > I think enabling regrename by default is the right thing to do long term. Then rs6000 (and many other ports probably) will just turn it off again. It is actively harmful. Segher
On 09/14/2016 03:04 PM, Segher Boessenkool wrote: > Then rs6000 (and many other ports probably) will just turn it off again. > It is actively harmful. The data that was posted showed a code size decrease on a number of targets. I'm really not sure where this irrational hate for regrename comes from. Bernd
On Wed, Sep 14, 2016 at 03:08:21PM +0200, Bernd Schmidt wrote: > On 09/14/2016 03:04 PM, Segher Boessenkool wrote: > >Then rs6000 (and many other ports probably) will just turn it off again. > >It is actively harmful. > > The data that was posted showed a code size decrease on a number of > targets. I'm really not sure where this irrational hate for regrename > comes from. It increases the number of active, "young" registers per thread. There is no irrational hate. Regrename is simply a de-optimisation on some (heavily) out-of-order targets. Not by much, but not a win either. We would rather do without it, on current CPUs at least (and I doubt this will change soon, but we'll see). (It also makes the generated code much harder to read, but you know that). Segher
On 09/14/2016 03:55 PM, Segher Boessenkool wrote: > On Wed, Sep 14, 2016 at 03:08:21PM +0200, Bernd Schmidt wrote: >> The data that was posted showed a code size decrease on a number of >> targets. I'm really not sure where this irrational hate for regrename >> comes from. > > It increases the number of active, "young" registers per thread. > > There is no irrational hate. Regrename is simply a de-optimisation on > some (heavily) out-of-order targets. Can you point me at a processor manual for such a chip that explains why this would be the case? > (It also makes the generated code much harder to read, but you know that). Can't say I do, really. I imagine it could be the case if it enables more aggressive scheduling but that's kind of one of the intended effects. Bernd
Hi Bernd, On Wed, Sep 14, 2016 at 04:01:34PM +0200, Bernd Schmidt wrote: > On 09/14/2016 03:55 PM, Segher Boessenkool wrote: > >On Wed, Sep 14, 2016 at 03:08:21PM +0200, Bernd Schmidt wrote: > >>The data that was posted showed a code size decrease on a number of > >>targets. I'm really not sure where this irrational hate for regrename > >>comes from. > > > >It increases the number of active, "young" registers per thread. > > > >There is no irrational hate. Regrename is simply a de-optimisation on > >some (heavily) out-of-order targets. > > Can you point me at a processor manual for such a chip that explains why > this would be the case? The POWER8 UM explains it a bit. I don't have an URL, everything is behind a registration wall it seems. > >(It also makes the generated code much harder to read, but you know that). > > Can't say I do, really. I imagine it could be the case if it enables > more aggressive scheduling but that's kind of one of the intended effects. We have 32 int regs, 32 float regs, 32 vec regs, 8 cond regs. It becomes really hard to read if things are pretty much randomly jumbled about, as regrename does. The dump files are almost impossible to read btw, regrename prints the assembler name of registers only (not the RTL name), and the assembler name of all of GPRn, FPRn, VRn, CRn is "n". All (?) other passes print the RTL register number (or both). Segher
On 09/14/2016 07:55 AM, Segher Boessenkool wrote: > On Wed, Sep 14, 2016 at 03:08:21PM +0200, Bernd Schmidt wrote: >> On 09/14/2016 03:04 PM, Segher Boessenkool wrote: >>> Then rs6000 (and many other ports probably) will just turn it off again. >>> It is actively harmful. >> >> The data that was posted showed a code size decrease on a number of >> targets. I'm really not sure where this irrational hate for regrename >> comes from. > > It increases the number of active, "young" registers per thread. Yea, it'll certainly do that and I can imagine a design which would have that property. And there's other designs which benefit from spreading across the register file as much as possible. Which argues there needs to be a way to tune or disable the pass. Jeff
On 09/14/2016 07:04 AM, Segher Boessenkool wrote: >> I'd >> probably start by fixing the dataflow issues and see if that fixes the >> regrename thing as a side effect. > > Have you seen https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00091.html ? I missed it. My interpretation.... The uses at each "strange" exit fixing the first issue with shrink-wrapping definitely sounds like we've got a dataflow problem of some sort. If you think about it, conceptually we want the return insn to make the callee saved registers "used" so that DCE, regrename and friends don't muck with them. The fact that we don't is as much never having to care all that much until recently. I continue to wonder if we could add something similar to CALL_INSN_FUNCTION_USAGE where we attach uses for all the call-saved registers onto a return insn. We would attach them at the end of prologue/epilogue generation and only attach those where were live somewhere in the code. By deferring that step until after prologue/epilogue generation we shouldn't cause unnecessary register saves/restores. I pondered just doing it for the separately wrapped components on that particular path, but I've yet to convince myself that's actually correct. Bernd knows the regrename code better than anyone. Is there any way the two of you could work together to try and track down what's going on in the hash_map_rand case? Even throwing in some more debug stuff might help narrow things down since it's renaming something to a non-volatile, non-separately shrink wrapped register that's causing problems. >> I think enabling regrename by default is the right thing to do long term. > > Then rs6000 (and many other ports probably) will just turn it off again. > It is actively harmful. I think you're over stating things here. Can we agree that there's a set of targets that will improve and a set that are harmed? And that to enable regrename by default we need to either better describe the pipeline characteristics we're optimizing for or a well defined way for targets to turn it off? jeff
On 09/14/2016 08:01 AM, Bernd Schmidt wrote: > On 09/14/2016 03:55 PM, Segher Boessenkool wrote: >> On Wed, Sep 14, 2016 at 03:08:21PM +0200, Bernd Schmidt wrote: >>> The data that was posted showed a code size decrease on a number of >>> targets. I'm really not sure where this irrational hate for regrename >>> comes from. >> >> It increases the number of active, "young" registers per thread. >> >> There is no irrational hate. Regrename is simply a de-optimisation on >> some (heavily) out-of-order targets. > > Can you point me at a processor manual for such a chip that explains why > this would be the case? Ponder a processor where the cost to access a register is non-uniform and related to how long ago a particular register was used. This can happen when the actual hardware register file is much larger than the exposed register file (to support hardware renaming, hyperthreading, partitioning, etc). > I imagine it could be the case if it enables > more aggressive scheduling but that's kind of one of the intended effects. Exactly. jeff
On Wed, Sep 14, 2016 at 12:11:50PM -0600, Jeff Law wrote: > On 09/14/2016 07:04 AM, Segher Boessenkool wrote: > >>I'd > >>probably start by fixing the dataflow issues and see if that fixes the > >>regrename thing as a side effect. > > > >Have you seen https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00091.html ? > I missed it. Yeah thought so, too much stuff in flight here. > My interpretation.... > > The uses at each "strange" exit fixing the first issue with > shrink-wrapping definitely sounds like we've got a dataflow problem of > some sort. > > If you think about it, conceptually we want the return insn to make the > callee saved registers "used" so that DCE, regrename and friends don't > muck with them. The fact that we don't is as much never having to care > all that much until recently. (There is no return insn at those exits; these are exits *without* successor block, not the exit block). It is puzzling to me why adding USEs for just the registers that *are* separately shrink-wrapped does not work; only all those that *could* be shrink-wrapped does. Do you have any idea about that? > I continue to wonder if we could add something similar to > CALL_INSN_FUNCTION_USAGE where we attach uses for all the call-saved > registers onto a return insn. We would attach them at the end of > prologue/epilogue generation and only attach those where were live > somewhere in the code. Maybe adding the new insns to the {pro,epi}logue_insn_hash will help something. Or maybe it will blow up spectacularly. Will know in a bit. > By deferring that step until after prologue/epilogue generation we > shouldn't cause unnecessary register saves/restores. Hrm. I'll see about that as well. > I pondered just doing it for the separately wrapped components on that > particular path, but I've yet to convince myself that's actually correct. If that is not correct, how is the status quo correct? That is what puzzles me above, too. > Bernd knows the regrename code better than anyone. Is there any way the > two of you could work together to try and track down what's going on in > the hash_map_rand case? Even throwing in some more debug stuff might > help narrow things down since it's renaming something to a non-volatile, > non-separately shrink wrapped register that's causing problems. Okay with me, I could certainly use his help. I'll try the above things first though, so not before friday. > Can we agree that there's a set of targets that will improve and a set > that are harmed? And that to enable regrename by default we need to > either better describe the pipeline characteristics we're optimizing for > or a well defined way for targets to turn it off? There is a well-defined way to turn it off, via common/config/*/*-common.c , TARGET_OPTION_OPTIMIZATION_TABLE. We disagree on whether most targets will want it enabled, i.e. whether it should (eventually) be enabled by default. Segher
On Wed, Sep 14, 2016 at 11:52:02AM -0600, Jeff Law wrote: > Yea, it'll certainly do that and I can imagine a design which would have > that property. And there's other designs which benefit from spreading > across the register file as much as possible. > > Which argues there needs to be a way to tune or disable the pass. Yes, some targets want it, and some don't. It seems to me that targets that use sched1 have much less benefit from regrename. Of course enabling sched1 has its own problems. Segher
On 09/14/2016 01:10 PM, Segher Boessenkool wrote: > On Wed, Sep 14, 2016 at 11:52:02AM -0600, Jeff Law wrote: >> Yea, it'll certainly do that and I can imagine a design which would have >> that property. And there's other designs which benefit from spreading >> across the register file as much as possible. >> >> Which argues there needs to be a way to tune or disable the pass. > > Yes, some targets want it, and some don't. It seems to me that targets > that use sched1 have much less benefit from regrename. Of course enabling > sched1 has its own problems. I think that sched1 would decrease the benefit of reg renaming, but wouldn't totally eliminate the desire to avoid anti dependencies created by register allocation. jeff
On 09/14/2016 01:03 PM, Segher Boessenkool wrote: >> If you think about it, conceptually we want the return insn to make the >> callee saved registers "used" so that DCE, regrename and friends don't >> muck with them. The fact that we don't is as much never having to care >> all that much until recently. > > (There is no return insn at those exits; these are exits *without* > successor block, not the exit block). Ugh. Anywhere we could attach this stuff in the insn chain? If not, the DF side of this problem gets uglier. > > It is puzzling to me why adding USEs for just the registers that *are* > separately shrink-wrapped does not work; only all those that *could* be > shrink-wrapped does. Do you have any idea about that? Nope. No clue on that one. It almost seems like a book-keeping error somewhere. > >> I continue to wonder if we could add something similar to >> CALL_INSN_FUNCTION_USAGE where we attach uses for all the call-saved >> registers onto a return insn. We would attach them at the end of >> prologue/epilogue generation and only attach those where were live >> somewhere in the code. > > Maybe adding the new insns to the {pro,epi}logue_insn_hash will help > something. Or maybe it will blow up spectacularly. Will know in a > bit. I think that'll resolve the sel-sched issue, but I doubt the rest. > >> I pondered just doing it for the separately wrapped components on that >> particular path, but I've yet to convince myself that's actually correct. > > If that is not correct, how is the status quo correct? That is what > puzzles me above, too. I couldn't convince myself that we could trivially find all the separately wrapped components on a particular path -- ie, we may have had components saved/restored in some sub-graph. If an exit point from the function is reachable from that sub-graph, then we need to make sure any components from the subgraph are marked as live in addition to those which are restored on the exit path. While it is just a reachability problem, I don't think we need to solve it if we mark anything that was separately shrink wrapped as live at all the exit points. > > There is a well-defined way to turn it off, via common/config/*/*-common.c , > TARGET_OPTION_OPTIMIZATION_TABLE. We disagree on whether most targets will > want it enabled, i.e. whether it should (eventually) be enabled by default. I'm aware of TARGET_OPTION_OPTIMIZATION_TABLE -- I was thinking more along the lines of trying to describe the conditions under which it is not profitable and expose that. TARGET_OPTION_OPTIMIZATION_TABLE is a lame way to attack this problem. Jeff
On 09/14/2016 01:03 PM, Segher Boessenkool wrote: > > (There is no return insn at those exits; these are exits *without* > successor block, not the exit block). Hmm, I thought these were return blocks, but you're saying they're no-return blocks? I missed that. In that case, aren't the restores dead because no callers can observe their value changing unexpectedly? Jeff
On Wed, Sep 14, 2016 at 01:33:04PM -0600, Jeff Law wrote: > On 09/14/2016 01:03 PM, Segher Boessenkool wrote: > >>If you think about it, conceptually we want the return insn to make the > >>callee saved registers "used" so that DCE, regrename and friends don't > >>muck with them. The fact that we don't is as much never having to care > >>all that much until recently. > > > >(There is no return insn at those exits; these are exits *without* > >successor block, not the exit block). > Ugh. Anywhere we could attach this stuff in the insn chain? If not, > the DF side of this problem gets uglier. I put the USEs at the start of that noreturn basic block. > >It is puzzling to me why adding USEs for just the registers that *are* > >separately shrink-wrapped does not work; only all those that *could* be > >shrink-wrapped does. Do you have any idea about that? > Nope. No clue on that one. It almost seems like a book-keeping error > somewhere. Right. > >>I pondered just doing it for the separately wrapped components on that > >>particular path, but I've yet to convince myself that's actually correct. > > > >If that is not correct, how is the status quo correct? That is what > >puzzles me above, too. > I couldn't convince myself that we could trivially find all the > separately wrapped components on a particular path -- ie, we may have > had components saved/restored in some sub-graph. If an exit point from > the function is reachable from that sub-graph, then we need to make sure > any components from the subgraph are marked as live in addition to those > which are restored on the exit path. > > While it is just a reachability problem, I don't think we need to solve > it if we mark anything that was separately shrink wrapped as live at all > the exit points. Agreed, but why does it work if not separately shrink-wrapping anything? And why does it break on things that are *not* separately wrapped *anywhere*? > >There is a well-defined way to turn it off, via common/config/*/*-common.c > >, > >TARGET_OPTION_OPTIMIZATION_TABLE. We disagree on whether most targets will > >want it enabled, i.e. whether it should (eventually) be enabled by default. > I'm aware of TARGET_OPTION_OPTIMIZATION_TABLE -- I was thinking more > along the lines of trying to describe the conditions under which it is > not profitable and expose that. TARGET_OPTION_OPTIMIZATION_TABLE is a > lame way to attack this problem. Better than a target macro ;-) Segher
On Wed, Sep 14, 2016 at 01:35:57PM -0600, Jeff Law wrote: > On 09/14/2016 01:03 PM, Segher Boessenkool wrote: > > > >(There is no return insn at those exits; these are exits *without* > >successor block, not the exit block). > Hmm, I thought these were return blocks, but you're saying they're > no-return blocks? I missed that. > > In that case, aren't the restores dead because no callers can observe > their value changing unexpectedly? Yes, but DCE can not remove the insns because dwarf2cfi would throw a fit (and for good reason). That is why adding all these USEs makes the DCE patch unnecessary (that patch simply disallows deleting insns with a REG_CFA_RESTORE note, much simpler than all that USE surgery, and perfectly correct and pretty much the best we can do). The problem is that regrename decides to rename a (volatile) register to some non-volatile register that is *not* separately shrink-wrapped (and not actually dead there). Why would it do that, why would it not do that if not separately shrink-wrapping at all? (I did run this with checking=yes,rtl btw, it is not simple corruption). Segher
On 09/14/2016 04:11 PM, Segher Boessenkool wrote: > On Wed, Sep 14, 2016 at 01:33:04PM -0600, Jeff Law wrote: >> On 09/14/2016 01:03 PM, Segher Boessenkool wrote: >>>> If you think about it, conceptually we want the return insn to make the >>>> callee saved registers "used" so that DCE, regrename and friends don't >>>> muck with them. The fact that we don't is as much never having to care >>>> all that much until recently. >>> >>> (There is no return insn at those exits; these are exits *without* >>> successor block, not the exit block). >> Ugh. Anywhere we could attach this stuff in the insn chain? If not, >> the DF side of this problem gets uglier. > > I put the USEs at the start of that noreturn basic block. Just naked USEs of the REG? For some reason I was uneasy about this, but I can't recall why, maybe I just latched onto CALL_INSN_FUNCTION_USAGE and wanted to use the same model. Seems like we should just go with the naked USE of the REGs. >> While it is just a reachability problem, I don't think we need to solve >> it if we mark anything that was separately shrink wrapped as live at all >> the exit points. > > Agreed, but why does it work if not separately shrink-wrapping anything? > And why does it break on things that are *not* separately wrapped *anywhere*? That I don't know... It's a hell of a mystery. Jeff
Hi Jeff, On Wed, Sep 14, 2016 at 01:33:04PM -0600, Jeff Law wrote: > On 09/14/2016 01:03 PM, Segher Boessenkool wrote: > >>If you think about it, conceptually we want the return insn to make the > >>callee saved registers "used" so that DCE, regrename and friends don't > >>muck with them. The fact that we don't is as much never having to care > >>all that much until recently. > > > >(There is no return insn at those exits; these are exits *without* > >successor block, not the exit block). > Ugh. Anywhere we could attach this stuff in the insn chain? If not, > the DF side of this problem gets uglier. > >>I continue to wonder if we could add something similar to > >>CALL_INSN_FUNCTION_USAGE where we attach uses for all the call-saved > >>registers onto a return insn. We would attach them at the end of > >>prologue/epilogue generation and only attach those where were live > >>somewhere in the code. > > > >Maybe adding the new insns to the {pro,epi}logue_insn_hash will help > >something. Or maybe it will blow up spectacularly. Will know in a > >bit. > I think that'll resolve the sel-sched issue, but I doubt the rest. Adding all the separately shrink-wrapped *logues to the hashes does not help at all. Trying to fix DF did not work either. > >>I pondered just doing it for the separately wrapped components on that > >>particular path, but I've yet to convince myself that's actually correct. > > > >If that is not correct, how is the status quo correct? That is what > >puzzles me above, too. > I couldn't convince myself that we could trivially find all the > separately wrapped components on a particular path -- ie, we may have > had components saved/restored in some sub-graph. If an exit point from > the function is reachable from that sub-graph, then we need to make sure > any components from the subgraph are marked as live in addition to those > which are restored on the exit path. > > While it is just a reachability problem, I don't think we need to solve > it if we mark anything that was separately shrink wrapped as live at all > the exit points. If I mark every block with no successors as needing all components, various patches are no longer needed. It also should not hurt performance much (with the possible exception of sibling calls, which can be executed often; still have to do performance runs). The dce and cprop patches and the first regrename patch are still needed. Those three are simple and obviously correct patches though. I'll walk through all your comments again, updating the patches. Will send a v3 soon. Thanks for all the reviews and helpful comments, Segher
diff --git a/gcc/regrename.c b/gcc/regrename.c index 00a5d02..5740b1a 100644 --- a/gcc/regrename.c +++ b/gcc/regrename.c @@ -1956,7 +1956,11 @@ public: /* opt_pass methods: */ virtual bool gate (function *) { - return (optimize > 0 && (flag_rename_registers)); + /* regrename creates wrong code for exception handling, if used together + with separate shrink-wrapping. Disable for now, until we have + figured out what exactly is going on. */ + return (optimize > 0 && flag_rename_registers + && !crtl->shrink_wrapped_separate); } virtual unsigned int execute (function *) { return regrename_optimize (); }