| Message ID | 1447087669-14039-11-git-send-email-tbsaunde+gcc@tbsaunde.org |
|---|---|
| State | New |
| Headers | show |
On Mon, 2015-11-09 at 11:47 -0500, tbsaunde+gcc@tbsaunde.org wrote: > From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org> > > gcc/ChangeLog: > > 2015-11-09 Trevor Saunders <tbsaunde+gcc@tbsaunde.org> > > * defaults.h (EH_RETURN_HANDLER_RTX): New default definition. > * df-scan.c (df_get_exit_block_use_set): Adjust. > * except.c (expand_eh_return): Likewise. > --- > gcc/defaults.h | 4 ++++ > gcc/df-scan.c | 2 -- > gcc/except.c | 9 ++++----- > 3 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/gcc/defaults.h b/gcc/defaults.h > index c20de44..047a0db 100644 > --- a/gcc/defaults.h > +++ b/gcc/defaults.h > @@ -1325,6 +1325,10 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see > #define TARGET_PECOFF 0 > #endif > > +#ifndef EH_RETURN_HANDLER_RTX > +#define EH_RETURN_HANDLER_RTX NULL > +#endif > + > #ifdef GCC_INSN_FLAGS_H > /* Dependent default target macro definitions > > diff --git a/gcc/df-scan.c b/gcc/df-scan.c > index 2e5fe97..a735925 100644 > --- a/gcc/df-scan.c > +++ b/gcc/df-scan.c > @@ -3714,7 +3714,6 @@ df_get_exit_block_use_set (bitmap exit_block_uses) > } > #endif > > -#ifdef EH_RETURN_HANDLER_RTX > if ((!targetm.have_epilogue () || ! epilogue_completed) > && crtl->calls_eh_return) > { > @@ -3722,7 +3721,6 @@ df_get_exit_block_use_set (bitmap exit_block_uses) > if (tmp && REG_P (tmp)) > df_mark_reg (tmp, exit_block_uses); > } > -#endif > > /* Mark function return value. */ > diddle_return_value (df_mark_reg, (void*) exit_block_uses); > diff --git a/gcc/except.c b/gcc/except.c > index 1801fe7..1a41a34 100644 > --- a/gcc/except.c > +++ b/gcc/except.c > @@ -2255,11 +2255,10 @@ expand_eh_return (void) > emit_insn (targetm.gen_eh_return (crtl->eh.ehr_handler)); > else > { > -#ifdef EH_RETURN_HANDLER_RTX > - emit_move_insn (EH_RETURN_HANDLER_RTX, crtl->eh.ehr_handler); > -#else > - error ("__builtin_eh_return not supported on this target"); > -#endif > + if (rtx handler = EH_RETURN_HANDLER_RTX) Would this be clearer as rtx handler = EH_RETURN_HANDLER_RTX; if (handler) ? (to avoid an assignment inside a conditional) > + emit_move_insn (handler, crtl->eh.ehr_handler); > + else > + error ("__builtin_eh_return not supported on this target"); > } > > emit_label (around_label);
On 11/09/2015 05:47 PM, tbsaunde+gcc@tbsaunde.org wrote: > From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org> > > gcc/ChangeLog: > > 2015-11-09 Trevor Saunders <tbsaunde+gcc@tbsaunde.org> > > * defaults.h (EH_RETURN_HANDLER_RTX): New default definition. > * df-scan.c (df_get_exit_block_use_set): Adjust. > * except.c (expand_eh_return): Likewise. As I said for a previous patch series, if we go to the trouble of fixing up stuff like this, we might as well do it properly and turn things like this into a target hook. Bernd
On 11/09/2015 11:27 AM, Bernd Schmidt wrote: > On 11/09/2015 05:47 PM, tbsaunde+gcc@tbsaunde.org wrote: >> From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org> >> >> gcc/ChangeLog: >> >> 2015-11-09 Trevor Saunders <tbsaunde+gcc@tbsaunde.org> >> >> * defaults.h (EH_RETURN_HANDLER_RTX): New default definition. >> * df-scan.c (df_get_exit_block_use_set): Adjust. >> * except.c (expand_eh_return): Likewise. > > As I said for a previous patch series, if we go to the trouble of fixing > up stuff like this, we might as well do it properly and turn things like > this into a target hook. I agree that pushing hookization further is good as well. I still think the patch in and of itself is a step forward, even if it doesn't hookize EH_RETURN_HANDLER_RTX. jeff
On 11/09/2015 07:42 PM, Jeff Law wrote: > On 11/09/2015 11:27 AM, Bernd Schmidt wrote: >> On 11/09/2015 05:47 PM, tbsaunde+gcc@tbsaunde.org wrote: >>> From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org> >>> >>> gcc/ChangeLog: >>> >>> 2015-11-09 Trevor Saunders <tbsaunde+gcc@tbsaunde.org> >>> >>> * defaults.h (EH_RETURN_HANDLER_RTX): New default definition. >>> * df-scan.c (df_get_exit_block_use_set): Adjust. >>> * except.c (expand_eh_return): Likewise. >> >> As I said for a previous patch series, if we go to the trouble of fixing >> up stuff like this, we might as well do it properly and turn things like >> this into a target hook. > I agree that pushing hookization further is good as well. I still think > the patch in and of itself is a step forward, even if it doesn't hookize > EH_RETURN_HANDLER_RTX. Well, I was hoping that, by pointing out the issue for the last patch set, the next set of patches would get things right. We really shouldn't make sideways steps when there's a simple way to go forward. Bernd
On Mon, Nov 09, 2015 at 11:42:19AM -0700, Jeff Law wrote: > On 11/09/2015 11:27 AM, Bernd Schmidt wrote: > >On 11/09/2015 05:47 PM, tbsaunde+gcc@tbsaunde.org wrote: > >>From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org> > >> > >>gcc/ChangeLog: > >> > >>2015-11-09 Trevor Saunders <tbsaunde+gcc@tbsaunde.org> > >> > >> * defaults.h (EH_RETURN_HANDLER_RTX): New default definition. > >> * df-scan.c (df_get_exit_block_use_set): Adjust. > >> * except.c (expand_eh_return): Likewise. > > > >As I said for a previous patch series, if we go to the trouble of fixing > >up stuff like this, we might as well do it properly and turn things like > >this into a target hook. > I agree that pushing hookization further is good as well. I still think the > patch in and of itself is a step forward, even if it doesn't hookize > EH_RETURN_HANDLER_RTX. yeah, that's more or less my thought, and this makes hookization easier since you can now mechanically add a hook for each thing in defaults.h that invokes the macro. Then for each target you can go through and replace the macro with an override of the hooks. That ends up with the macros replaced by hooks without writing a lot of patches that need to go through config-list.mk, and testing on multiple targets which imho is a giant pain, and rather slow. Trev > > jeff
On 11/09/2015 07:52 PM, Trevor Saunders wrote: > yeah, that's more or less my thought, and this makes hookization easier > since you can now mechanically add a hook for each thing in defaults.h > that invokes the macro. Then for each target you can go through and > replace the macro with an override of the hooks. That ends up with the > macros replaced by hooks without writing a lot of patches that need to > go through config-list.mk, and testing on multiple targets which imho is > a giant pain, and rather slow. We might want to think about making a policy decision to try waiving some of the testing requirements for target macro -> hook conversions. Maybe try only a "build to cc1" requirement and see whether that causes too much breakage. Bernd
On 11/09/2015 12:38 PM, Bernd Schmidt wrote: > On 11/09/2015 07:52 PM, Trevor Saunders wrote: > >> yeah, that's more or less my thought, and this makes hookization easier >> since you can now mechanically add a hook for each thing in defaults.h >> that invokes the macro. Then for each target you can go through and >> replace the macro with an override of the hooks. That ends up with the >> macros replaced by hooks without writing a lot of patches that need to >> go through config-list.mk, and testing on multiple targets which imho is >> a giant pain, and rather slow. > > We might want to think about making a policy decision to try waiving > some of the testing requirements for target macro -> hook conversions. > Maybe try only a "build to cc1" requirement and see whether that causes > too much breakage. A config-list.mk build is a build to cc1*, f951, gnat1, so we're not requiring deep tests on the affected targets. Not sure how much we're getting by forcing a bootstrap & regression test of that kind of change. I'm certainly open to this kind of relaxed testing to help this stuff move forward an complete before we're all retired :-) Jeff
On Nov 9, 2015, at 11:46 AM, Jeff Law <law@redhat.com> wrote: On 11/09/2015 12:38 PM, Bernd Schmidt wrote: >> We might want to think about making a policy decision to try waiving >> some of the testing requirements for target macro -> hook conversions. >> Maybe try only a "build to cc1" requirement and see whether that causes >> too much breakage. > A config-list.mk build is a build to cc1*, f951, gnat1, so we're not requiring deep tests on the affected targets. Not sure how much we're getting by forcing a bootstrap & regression test of that kind of change. > > I'm certainly open to this kind of relaxed testing to help this stuff move forward an complete before we're all retired :-) Testing is a cornerstone of gcc quality. I like it. It is useful. That said, I don’t think we should always be fanatical about it. How and when we accept less that a standard bootstrap and regression test run I’ve sure would be a big topic, but rather than make a ton of rules, I’d rather let small handful of reviewers decide when and how to accept less, and let them do what they want. We can give them negative feedback if it impacts too many people, too often and they can adjust. The other way, would be to have an integration branch that is tested and merged post testing on a regular basis and let people contribute less than well tested things on it, the idea being that it still won’t hit trunk until after a bootstrap and tests suite run, but that we can bundle 2-100 patches into one test suite run. This strikes me as more scalable, easier for developers and removes the requirement of test suite + bootstrap before checkin while retaining the useful quality of everything merged to trunk is tested. Hardest part about this would be ChangeLogs, merge resolution and svn blame. git handles this gracefully. svn as I recall, a little less so. [ quick check ] Ah, seems svn blame -g TARGET ca n handle this graceful (in theory).
On Mon, Nov 09, 2015 at 12:46:33PM -0700, Jeff Law wrote: > On 11/09/2015 12:38 PM, Bernd Schmidt wrote: > >On 11/09/2015 07:52 PM, Trevor Saunders wrote: > > > >>yeah, that's more or less my thought, and this makes hookization easier > >>since you can now mechanically add a hook for each thing in defaults.h > >>that invokes the macro. Then for each target you can go through and > >>replace the macro with an override of the hooks. That ends up with the > >>macros replaced by hooks without writing a lot of patches that need to > >>go through config-list.mk, and testing on multiple targets which imho is > >>a giant pain, and rather slow. > > > >We might want to think about making a policy decision to try waiving > >some of the testing requirements for target macro -> hook conversions. > >Maybe try only a "build to cc1" requirement and see whether that causes > >too much breakage. > A config-list.mk build is a build to cc1*, f951, gnat1, so we're not > requiring deep tests on the affected targets. Not sure how much we're > getting by forcing a bootstrap & regression test of that kind of change. So in general when I've done cross target things I think I've found more bugs with config-list.mk than with a regtest, but the regtest has found some things I think. However I actually don't mind bootstrapping and regtesting that much, its more or less a few hours for the control and then another few for each patch. On the other hand config-list.mk takes on the order of 12 hours, and setting up a cross for a quick test isn't really that quick. Which means that if you have a patch touching a number of targets you end up not checking it compiles at all until you run config-list.mk, and then its a heavy weight operation. So at least for the way I work I'd really rather write series that I can incrementally test on just one target and be reasonably confident they won't break other targets. The add default macro definitions then wrap those with hooks, then target by target replace the macro by hook overrides approach seems to provide that you can incrementally test and fiind most of the issues, but the change a macro every where approach doesn't really. Trev The add default macros then use those in hooks, and finally add overides > > I'm certainly open to this kind of relaxed testing to help this stuff move > forward an complete before we're all retired :-) > > Jeff >
On 11/09/2015 02:30 PM, Trevor Saunders wrote: > > So in general when I've done cross target things I think I've found more > bugs with config-list.mk than with a regtest, but the regtest has found > some things I think. I'm finding config-list.mk fairly reliable, with the notable exception of the avr-rtems issue and interix. But that may simply be function of running it regularly. > > However I actually don't mind bootstrapping and regtesting that much, > its more or less a few hours for the control and then another few for > each patch. I usually save my results and only go back for a control build if something goes wrong. Of course I'm usually stepping forward at least once a day, so the number of new tests is usually manageable and allows me to compare the first run of the day with the last run of the prior day. On the other hand config-list.mk takes on the order of 12 > hours, and setting up a cross for a quick test isn't really that quick. > Which means that if you have a patch touching a number of targets you > end up not checking it compiles at all until you run config-list.mk, and > then its a heavy weight operation. FWIW, If we know what ports a particular patch would hit, I'd fully support folks doing builds that didn't hit all of config-list.mk. In case it's not obvious I do hope that we'll get to a point where the class of bugs like "X is unused on port PDQ because it defines/does not define FROBIT" just go away and we can get good first level coverage with a native and perhaps a very small number of crosses (instead of the 200+ in config-list.mk now). At some point I also want to see config-list.mk extended to do things like "build the crosses and run test tree-ssa/ssa-dom-thread-11.c on all of them". I've got hacks to do that locally, but they're strictly hacks. I think this selectively deeper testing will become more important as we put the first level coverage behind us. > > So at least for the way I work I'd really rather write series that I can > incrementally test on just one target and be reasonably confident they > won't break other targets. That generally works for me. > > The add default macro definitions then wrap those with hooks, then > target by target replace the macro by hook overrides approach seems to > provide that you can incrementally test and fiind most of the issues, > but the change a macro every where approach doesn't really. I think Bernd and I just have different approaches, preferences and priorities on some stuff which results in slightly different priorities or approaches to certain issues. I've known Bernd a long time and will say he's very reasonable and his concerns/objections are well thought out and carry a ton of weight with me. Jeff
On Mon, Nov 09, 2015 at 03:07:21PM -0700, Jeff Law wrote: > On 11/09/2015 02:30 PM, Trevor Saunders wrote: > > > >So in general when I've done cross target things I think I've found more > >bugs with config-list.mk than with a regtest, but the regtest has found > >some things I think. > I'm finding config-list.mk fairly reliable, with the notable exception of > the avr-rtems issue and interix. But that may simply be function of running > it regularly. yeah, its reliable although I tend to find needing to have an installed trunk compiler a little painful. What I meant was that sometimes I've made mistakes and introduced testsuite failures or the like. > > > >However I actually don't mind bootstrapping and regtesting that much, > >its more or less a few hours for the control and then another few for > >each patch. > I usually save my results and only go back for a control build if something > goes wrong. Of course I'm usually stepping forward at least once a day, so > the number of new tests is usually manageable and allows me to compare the > first run of the day with the last run of the prior day. SO my usual mode is to build up a branch just checking that a default build works, and then at the end run a script that regtests all the patches. That can suffer from intermitant tests, but its low human input so I like it. > On the other hand config-list.mk takes on the order of 12 > >hours, and setting up a cross for a quick test isn't really that quick. > >Which means that if you have a patch touching a number of targets you > >end up not checking it compiles at all until you run config-list.mk, and > >then its a heavy weight operation. > FWIW, If we know what ports a particular patch would hit, I'd fully support > folks doing builds that didn't hit all of config-list.mk. sure > In case it's not obvious I do hope that we'll get to a point where the class > of bugs like "X is unused on port PDQ because it defines/does not define > FROBIT" just go away and we can get good first level coverage with a native > and perhaps a very small number of crosses (instead of the 200+ in > config-list.mk now). > > At some point I also want to see config-list.mk extended to do things like > "build the crosses and run test tree-ssa/ssa-dom-thread-11.c on all of > them". I've got hacks to do that locally, but they're strictly hacks. I > think this selectively deeper testing will become more important as we put > the first level coverage behind us. yeah, I'd actually like to see config-list.mk become part of the "real" build system at some point and you could do something like ./configure --target=i686-linux-gnu,ppc64-linux-gnu,alpha-dec-vms and stuff. > >So at least for the way I work I'd really rather write series that I can > >incrementally test on just one target and be reasonably confident they > >won't break other targets. > That generally works for me. > > > > >The add default macro definitions then wrap those with hooks, then > >target by target replace the macro by hook overrides approach seems to > >provide that you can incrementally test and fiind most of the issues, > >but the change a macro every where approach doesn't really. > I think Bernd and I just have different approaches, preferences and > priorities on some stuff which results in slightly different priorities or > approaches to certain issues. Sure, we're all different ;) > I've known Bernd a long time and will say he's very reasonable and his > concerns/objections are well thought out and carry a ton of weight with me. I don't really know him, but I don't really disagree with where he wants to get to. However I think we work fairly different ways, and review things differently. When I review patches (mostly for stuff more directly related to Mozilla my standards are basically it needs to be an improvement, and it needs to not introduce bugs. So I find the it might improve things, but it doesn't also accomplish X to berather odd, and hard to work with if I think getting directly to X might be hard. Trev > > Jeff
On Mon, 9 Nov 2015, Trevor Saunders wrote: > The add default macro definitions then wrap those with hooks, then > target by target replace the macro by hook overrides approach seems to > provide that you can incrementally test and fiind most of the issues, > but the change a macro every where approach doesn't really. I have this notion that once a target macro is "regular" enough - not used in code built for the target, not used in driver code, not used directly or indirectly in #if conditions except for the single default definition in defaults.h, target definitions only depend on the target architecture and not OS or other variations - it ought to be possible to do the conversion to a hook with some kind of automated refactoring tool (possibly with a little editing of its results). And so this sort of regularizing of target macros is helpful because it increases the number of target macros that could be converted in an automated manner.
On 11/09/2015 11:53 PM, Trevor Saunders wrote: > I don't really know him, but I don't really disagree with where he wants > to get to. However I think we work fairly different ways, and review > things differently. When I review patches (mostly for stuff more > directly related to Mozilla my standards are basically it needs to be an > improvement, and it needs to not introduce bugs. What I like to ask for is to not just make mechanical changes, but to apply a bit of thought while doing so. Often something better can be done with the same amount of effort (e.g. if one notices that the X_POINTER_IS_Y_POINTER macros are pointless). Such small oddities in the code add up over time, and it is better to eliminate them when making a change anyway. > So I find the it might > improve things, but it doesn't also accomplish X to berather odd, and > hard to work with if I think getting directly to X might be hard. In cases where getting to X is hard I'd agree, and the EH_RETURN_HANDLER_RTX thing isn't the best example since it's defined by many ports. What disappointed me in the previous patch series were cases like INITIAL_FRAME_ADDRESS_RTX which is used by only one port. In such a case I don't think it's unreasonable to point out that we actually want to get rid of these macros rather than going through an intermediate stage. Since EH_RETURN_HANDLER_RTX is a slightly different case, and Joseph said he sees value in the patch, it is OK. Bernd
On Tue, Nov 10, 2015 at 11:02:04PM +0100, Bernd Schmidt wrote: > On 11/09/2015 11:53 PM, Trevor Saunders wrote: > >I don't really know him, but I don't really disagree with where he wants > >to get to. However I think we work fairly different ways, and review > >things differently. When I review patches (mostly for stuff more > >directly related to Mozilla my standards are basically it needs to be an > >improvement, and it needs to not introduce bugs. > > What I like to ask for is to not just make mechanical changes, but to apply > a bit of thought while doing so. Often something better can be done with the > same amount of effort (e.g. if one notices that the X_POINTER_IS_Y_POINTER > macros are pointless). Such small oddities in the code add up over time, and > it is better to eliminate them when making a change anyway. yeah, I kind of have mixed feelings about that, on one hand its stuff that really should be cleaned up. ON the other hand making it non mechanical makes it more work than it would be as a mechanical replacement. I guess it depends how much you value computer time testing vs human time thinking or something. Also "that seems fine, but I see we could clean up X" seems nicer to me than "there's nothing wrong with this, but you might as well do X while your there", but maybe that's just habbit or something silly. > > So I find the it might > >improve things, but it doesn't also accomplish X to berather odd, and > >hard to work with if I think getting directly to X might be hard. > > In cases where getting to X is hard I'd agree, and the EH_RETURN_HANDLER_RTX > thing isn't the best example since it's defined by many ports. What > disappointed me in the previous patch series were cases like > INITIAL_FRAME_ADDRESS_RTX which is used by only one port. In such a case I > don't think it's unreasonable to point out that we actually want to get rid > of these macros rather than going through an intermediate stage. > > Since EH_RETURN_HANDLER_RTX is a slightly different case, and Joseph said he > sees value in the patch, it is OK. Well, I guess if you believe Joseph is right about automating conversion from macros to hooks there's an argument to be made that the intermediate stage is just fine even for macros with one definition, because it'll just be two totally mechanical steps. I'm not totally convinced it'll be easy to automate, but even if its a human making the work mechanical in my mind makes it easier. On the other hand for something like BIGGEST_FIELD_ALIGNMENT and ADJUST_FIELD_ALIGN it seems worth while to take a look at merging those into one hook (of course after the libojbc stuff is sorted). Trev > > > Bernd
diff --git a/gcc/defaults.h b/gcc/defaults.h index c20de44..047a0db 100644 --- a/gcc/defaults.h +++ b/gcc/defaults.h @@ -1325,6 +1325,10 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see #define TARGET_PECOFF 0 #endif +#ifndef EH_RETURN_HANDLER_RTX +#define EH_RETURN_HANDLER_RTX NULL +#endif + #ifdef GCC_INSN_FLAGS_H /* Dependent default target macro definitions diff --git a/gcc/df-scan.c b/gcc/df-scan.c index 2e5fe97..a735925 100644 --- a/gcc/df-scan.c +++ b/gcc/df-scan.c @@ -3714,7 +3714,6 @@ df_get_exit_block_use_set (bitmap exit_block_uses) } #endif -#ifdef EH_RETURN_HANDLER_RTX if ((!targetm.have_epilogue () || ! epilogue_completed) && crtl->calls_eh_return) { @@ -3722,7 +3721,6 @@ df_get_exit_block_use_set (bitmap exit_block_uses) if (tmp && REG_P (tmp)) df_mark_reg (tmp, exit_block_uses); } -#endif /* Mark function return value. */ diddle_return_value (df_mark_reg, (void*) exit_block_uses); diff --git a/gcc/except.c b/gcc/except.c index 1801fe7..1a41a34 100644 --- a/gcc/except.c +++ b/gcc/except.c @@ -2255,11 +2255,10 @@ expand_eh_return (void) emit_insn (targetm.gen_eh_return (crtl->eh.ehr_handler)); else { -#ifdef EH_RETURN_HANDLER_RTX - emit_move_insn (EH_RETURN_HANDLER_RTX, crtl->eh.ehr_handler); -#else - error ("__builtin_eh_return not supported on this target"); -#endif + if (rtx handler = EH_RETURN_HANDLER_RTX) + emit_move_insn (handler, crtl->eh.ehr_handler); + else + error ("__builtin_eh_return not supported on this target"); } emit_label (around_label);
From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org> gcc/ChangeLog: 2015-11-09 Trevor Saunders <tbsaunde+gcc@tbsaunde.org> * defaults.h (EH_RETURN_HANDLER_RTX): New default definition. * df-scan.c (df_get_exit_block_use_set): Adjust. * except.c (expand_eh_return): Likewise. --- gcc/defaults.h | 4 ++++ gcc/df-scan.c | 2 -- gcc/except.c | 9 ++++----- 3 files changed, 8 insertions(+), 7 deletions(-)