| Message ID | 1ee03bc0-ad1a-46dd-87bb-8288e990ce20@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Deprecating cc0 (and consequently cc0 targets) | expand |
On September 20, 2019 5:38:38 PM GMT+02:00, Jeff Law <law@redhat.com> wrote: >At the register allocation BOF during the Cauldron someone (I forget >who) raised the question of when/how do we get rid of reload. > >The first step in that process is to drop support for cc0. cc0 is a >horribly antiquated mechanism for describing how to handle condition >codes. It has numerous limitations, the most problematical being the >requirement that the cc0 setter and cc0 user must be consecutive in the >IL. This requirement bleeds all over the compiler resulting in code >that is harder to understand and maintain. I'm fairly confident this >code is broken in various ways, particularly WRT exceptions. > >So this message is serving as official notice that we are *deprecating* >all cc0 support in gcc-10. We are not removing any code or targets at >this time -- removals would happen during the gcc-11 cycle. > > >avr >cris >h8300 >m68k >vax >cr16 > >This is based on actually looking at the backend patterns. >backends.html indicates the mn103 port would be affected, but I think >it's just out-of-date. Similarly it fails to mention cr16, but cr16 >clearly has affected patterns (I'll address that separately) > >This patch deprecates the affected targets. With some magic folks can >continue to build them. However, if nobody steps forward to convert >them from cc0 to MODE_CC those targets will be removed during the >gcc-11 >cycle. > >If someone is interested in possibly doing a conversion, I would >strongly recommend reviewing the best documentation we have on the >subject: > >https://gcc.gnu.org/wiki/CC0Transition > >I worked with my son about a year ago to transition the v850 to MODE_CC >using that page as a guideline. We also have a partial transition for >the H8/300 port (not far enough to even build anything yet). I had >hoped we would finish the H8/300 port last year and would have tackled >another, but events have gotten in the way. > >The transition isn't terribly hard, but does take time because of the >number of patterns that have to be changed. > >Let the discussion begin... It seems to be that for the goal to keep a target alive a variant #2 conversion (according to the wiki) should be closely mirror CC0 behavior and thus should be easier to achieve and with less patterns touched than the 'good' variant. Can you acknowledge that and would such 'simple' conversions be OK? Richard. > >Jeff
I think the m68k-*-uclinuxoldabi* and vax-*-vms* cases need to move up to the top of that case statement (duplicating the "not supported" error), to keep that error even in the --enable-obsolete case. I see tile* targets are referenced in the diff context. They were obsoleted in GCC 8 (and have also been removed from glibc and the Linux kernel), isn't it time to remove them?
On 9/20/19 11:22 AM, Richard Biener wrote: > On September 20, 2019 5:38:38 PM GMT+02:00, Jeff Law <law@redhat.com> > wrote: >> At the register allocation BOF during the Cauldron someone (I >> forget who) raised the question of when/how do we get rid of >> reload. >> >> The first step in that process is to drop support for cc0. cc0 is >> a horribly antiquated mechanism for describing how to handle >> condition codes. It has numerous limitations, the most >> problematical being the requirement that the cc0 setter and cc0 >> user must be consecutive in the IL. This requirement bleeds all >> over the compiler resulting in code that is harder to understand >> and maintain. I'm fairly confident this code is broken in various >> ways, particularly WRT exceptions. >> >> So this message is serving as official notice that we are >> *deprecating* all cc0 support in gcc-10. We are not removing any >> code or targets at this time -- removals would happen during the >> gcc-11 cycle. >> >> >> avr cris h8300 m68k vax cr16 >> >> This is based on actually looking at the backend patterns. >> backends.html indicates the mn103 port would be affected, but I >> think it's just out-of-date. Similarly it fails to mention cr16, >> but cr16 clearly has affected patterns (I'll address that >> separately) >> >> This patch deprecates the affected targets. With some magic folks >> can continue to build them. However, if nobody steps forward to >> convert them from cc0 to MODE_CC those targets will be removed >> during the gcc-11 cycle. >> >> If someone is interested in possibly doing a conversion, I would >> strongly recommend reviewing the best documentation we have on the >> subject: >> >> https://gcc.gnu.org/wiki/CC0Transition >> >> I worked with my son about a year ago to transition the v850 to >> MODE_CC using that page as a guideline. We also have a partial >> transition for the H8/300 port (not far enough to even build >> anything yet). I had hoped we would finish the H8/300 port last >> year and would have tackled another, but events have gotten in the >> way. >> >> The transition isn't terribly hard, but does take time because of >> the number of patterns that have to be changed. >> >> Let the discussion begin... > > It seems to be that for the goal to keep a target alive a variant #2 > conversion (according to the wiki) should be closely mirror CC0 > behavior and thus should be easier to achieve and with less patterns > touched than the 'good' variant. Can you acknowledge that and would > such 'simple' conversions be OK? Unless the target has condition code preserving arithmetic a variant #2 conversion is the only way to go. Now if someone did a variant #2 without the optimization bits (ie, adding appropriate _set_flags pattern variants), I think that should be considered explicitly OK even though the code quality would potentially suffer. Essentially it's a choice between dropping the port or keeping the port, but with slightly less optimization. THe latter seems like a better choice to me. jeff
> On Sep 20, 2019, at 1:45 PM, Jeff Law <law@redhat.com> wrote: > > On 9/20/19 11:22 AM, Richard Biener wrote: >> ... >> It seems to be that for the goal to keep a target alive a variant #2 >> conversion (according to the wiki) should be closely mirror CC0 >> behavior and thus should be easier to achieve and with less patterns >> touched than the 'good' variant. Can you acknowledge that and would >> such 'simple' conversions be OK? > Unless the target has condition code preserving arithmetic a variant #2 > conversion is the only way to go. Yes. > Now if someone did a variant #2 without the optimization bits (ie, > adding appropriate _set_flags pattern variants), I think that should be > considered explicitly OK even though the code quality would potentially > suffer. Essentially it's a choice between dropping the port or keeping > the port, but with slightly less optimization. THe latter seems like a > better choice to me. True. Then again, the added work of creating the pattern pairs is modest. With define_subst, much of the work can be automated. For pdp11, I found this to be the case; the hard part was to learn what is needed, and for that the Wiki ends up a big help. Also, pdp11 is harder than most because it has two CC registers (one for float ops, one for the rest). I don't know all the others, but for example VAX only has one, and I'm pretty sure the same applies to m68k. paul
On 9/21/19 2:48 PM, Paul Koning wrote: > > >> On Sep 20, 2019, at 1:45 PM, Jeff Law <law@redhat.com> wrote: >> >> On 9/20/19 11:22 AM, Richard Biener wrote: >>> ... It seems to be that for the goal to keep a target alive a >>> variant #2 conversion (according to the wiki) should be closely >>> mirror CC0 behavior and thus should be easier to achieve and with >>> less patterns touched than the 'good' variant. Can you >>> acknowledge that and would such 'simple' conversions be OK? >> Unless the target has condition code preserving arithmetic a >> variant #2 conversion is the only way to go. > > Yes. > >> Now if someone did a variant #2 without the optimization bits (ie, >> adding appropriate _set_flags pattern variants), I think that >> should be considered explicitly OK even though the code quality >> would potentially suffer. Essentially it's a choice between >> dropping the port or keeping the port, but with slightly less >> optimization. THe latter seems like a better choice to me. > > True. Then again, the added work of creating the pattern pairs is > modest. With define_subst, much of the work can be automated. The patterns and support to handle optimization can be added after the basic conversion is done. In fact, that's precisely how I'd approach it. > > For pdp11, I found this to be the case; the hard part was to learn > what is needed, and for that the Wiki ends up a big help. Also, > pdp11 is harder than most because it has two CC registers (one for > float ops, one for the rest). I don't know all the others, but for > example VAX only has one, and I'm pretty sure the same applies to > m68k. m68k is like pdp11 in this regard. Two condition code registers, one in the main processor and another for the 68881 FP unit. jeff
On Sat, Sep 21, 2019 at 03:04:26PM -0600, Jeff Law wrote: > On 9/21/19 2:48 PM, Paul Koning wrote: > >> On Sep 20, 2019, at 1:45 PM, Jeff Law <law@redhat.com> wrote: > >> On 9/20/19 11:22 AM, Richard Biener wrote: > >> Now if someone did a variant #2 without the optimization bits (ie, > >> adding appropriate _set_flags pattern variants), I think that > >> should be considered explicitly OK even though the code quality > >> would potentially suffer. Essentially it's a choice between > >> dropping the port or keeping the port, but with slightly less > >> optimization. THe latter seems like a better choice to me. > > > > True. Then again, the added work of creating the pattern pairs is > > modest. With define_subst, much of the work can be automated. > The patterns and support to handle optimization can be added after the > basic conversion is done. In fact, that's precisely how I'd approach it. Yeah, but a type #2 conversion is more than that; it makes it harder to do a type #1 conversion later than if you started with doing just that. Of course it is better than totally dropping a target. Some coordination would be useful. OTOH, a type #2 conversion seems easy enough that maybe we can just pull that off for *all* targets for GCC 10, and actually remove CC0 already? > > For pdp11, I found this to be the case; the hard part was to learn > > what is needed, and for that the Wiki ends up a big help. Also, > > pdp11 is harder than most because it has two CC registers (one for > > float ops, one for the rest). I don't know all the others, but for > > example VAX only has one, and I'm pretty sure the same applies to > > m68k. > m68k is like pdp11 in this regard. Two condition code registers, one in > the main processor and another for the 68881 FP unit. I think the main difficulty with m68k is that it is a pretty big target. Segher
On Fri, Sep 20, 2019 at 09:38:38AM -0600, Jeff Law wrote:
> this time -- removals would happen during the gcc-11 cycle.
Hi Jeff,
I'm concerned that if I don't reach this milestone for VAX, it'll mean
that future code review will require justifying some of the original
changes which is getting increasingly challenging.
My first attempt at a CCmode conversion crashes early, and I was
suggested to first address any known bugs. I'll take another shot at it.
On September 22, 2019 1:48:34 AM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote: >On Sat, Sep 21, 2019 at 03:04:26PM -0600, Jeff Law wrote: >> On 9/21/19 2:48 PM, Paul Koning wrote: >> >> On Sep 20, 2019, at 1:45 PM, Jeff Law <law@redhat.com> wrote: >> >> On 9/20/19 11:22 AM, Richard Biener wrote: >> >> Now if someone did a variant #2 without the optimization bits (ie, > >> >> adding appropriate _set_flags pattern variants), I think that >> >> should be considered explicitly OK even though the code quality >> >> would potentially suffer. Essentially it's a choice between >> >> dropping the port or keeping the port, but with slightly less >> >> optimization. THe latter seems like a better choice to me. >> > >> > True. Then again, the added work of creating the pattern pairs is >> > modest. With define_subst, much of the work can be automated. >> The patterns and support to handle optimization can be added after >the >> basic conversion is done. In fact, that's precisely how I'd approach >it. > >Yeah, but a type #2 conversion is more than that; it makes it harder to >do a type #1 conversion later than if you started with doing just that. >Of course it is better than totally dropping a target. Some >coordination >would be useful. > >OTOH, a type #2 conversion seems easy enough that maybe we can just >pull >that off for *all* targets for GCC 10, and actually remove CC0 already? That was exactly my thinking... >> > For pdp11, I found this to be the case; the hard part was to learn >> > what is needed, and for that the Wiki ends up a big help. Also, >> > pdp11 is harder than most because it has two CC registers (one for >> > float ops, one for the rest). I don't know all the others, but for >> > example VAX only has one, and I'm pretty sure the same applies to >> > m68k. >> m68k is like pdp11 in this regard. Two condition code registers, one >in >> the main processor and another for the 68881 FP unit. > >I think the main difficulty with m68k is that it is a pretty big >target. > > >Segher
Hello! On September 20, 2019 5:38:38 PM GMT+02:00, Jeff Law <law@redhat.com> wrote: >So this message is serving as official notice that we are *deprecating* >all cc0 support in gcc-10. We are not removing any code or targets at >this time -- removals would happen during the gcc-11 cycle. > > >avr >cris >h8300 >m68k >vax >cr16 > >This is based on actually looking at the backend patterns. >backends.html indicates the mn103 port would be affected, but I think >it's just out-of-date. Similarly it fails to mention cr16, but cr16 >clearly has affected patterns (I'll address that separately) > >This patch deprecates the affected targets. With some magic folks can >continue to build them. However, if nobody steps forward to convert >them from cc0 to MODE_CC those targets will be removed during the >gcc-11 >cycle. We have raised $5000 to support anyone willing to work on this for the m68k target [1]. We really need the m68k to stay as it's essential to be able to compile for Linux/m68k, NetBSD/m68k and AROS/m68k (a new and ABI-compatible AmigaOS clone). I'm sure other communities like in the NeXTStep, Atari and Mac/m68k forums are interested in keeping the backend as well. I'm confident we will be able to raise even more money as the community is large and active. >If someone is interested in possibly doing a conversion, I would >strongly recommend reviewing the best documentation we have on the >subject: > >https://gcc.gnu.org/wiki/CC0Transition > >I worked with my son about a year ago to transition the v850 to MODE_CC >using that page as a guideline. We also have a partial transition for >the H8/300 port (not far enough to even build anything yet). I had >hoped we would finish the H8/300 port last year and would have tackled >another, but events have gotten in the way. Any chance that the unfinished code can be shared? I'm looking for any blueprints that can be used as a guidance for the work on the m68k backend. I have also created a guide on how to set up a QEMU virtual machine running a fully fledged Debian/m68k which can be used for the development work [2]. >The transition isn't terribly hard, but does take time because of the >number of patterns that have to be changed. I have already looked at the conversion done on the V850 backend [3] but so far I don't understand enough of the register representation stuff to be able to know where to start. I'm seeing that all the "cc" attributes are being removed but not replaced? Adrian > [1] https://www.bountysource.com/issues/80706251-m68k-convert-the-backend-to-mode_cc-so-it-can-be-kept-in-future-releases > [2] https://wiki.debian.org/M68k/QemuSystemM68k > [3] https://github.com/gcc-mirror/gcc/commit/d6c5e987e730b3b2b9ff396d2361518ff9cb5e23
On 10/29/19 6:26 AM, John Paul Adrian Glaubitz wrote: > Hello! > > We have raised $5000 to support anyone willing to work on this for the > m68k target [1]. We really need the m68k to stay as it's essential to > be able to compile for Linux/m68k, NetBSD/m68k and AROS/m68k (a new > and ABI-compatible AmigaOS clone). I'm sure other communities like in the > NeXTStep, Atari and Mac/m68k forums are interested in keeping the backend > as well. I'm confident we will be able to raise even more money as the > community is large and active. > >> If someone is interested in possibly doing a conversion, I would >> strongly recommend reviewing the best documentation we have on the >> subject: >> >> https://gcc.gnu.org/wiki/CC0Transition >> >> I worked with my son about a year ago to transition the v850 to MODE_CC >> using that page as a guideline. We also have a partial transition for >> the H8/300 port (not far enough to even build anything yet). I had >> hoped we would finish the H8/300 port last year and would have tackled >> another, but events have gotten in the way. > > Any chance that the unfinished code can be shared? I'm looking for any > blueprints that can be used as a guidance for the work on the m68k > backend. I'm not sure it'd be all that useful. The v850 bits would be a better starting point. The partial H8 transition isn't committed anywhere because it would certainly break the port. > > I have also created a guide on how to set up a QEMU virtual machine running > a fully fledged Debian/m68k which can be used for the development work [2]. > >> The transition isn't terribly hard, but does take time because of the >> number of patterns that have to be changed. > > I have already looked at the conversion done on the V850 backend [3] but so far > I don't understand enough of the register representation stuff to be able > to know where to start. I'm seeing that all the "cc" attributes are being removed > but not replaced? Right. The cc attributes were used to describe which condition codes were valid after each insn. That information now has to be carried by the mode of the condition code register. I'll walk you through one example from the H8 port. This isn't complete enough to even test that it builds. > (define_insn "*addsi_h8300" > [(set (match_operand:SI 0 "register_operand" "=r,r") > (plus:SI (match_operand:SI 1 "register_operand" "%0,0") > (match_operand:SI 2 "h8300_src_operand" "n,r")))] > "TARGET_H8300" > { > return output_plussi (operands); > } > [(set (attr "length") > (symbol_ref "compute_plussi_length (operands)")) > (set (attr "cc") > (symbol_ref "compute_plussi_cc (operands)"))]) That needs to turn into a define_insn_and_split. The idea is the form above is what's seen prior to register allocation and reloading. After reloading we turn it into a form with an attached clobber. We achieve this by rewriting the pattern into a define_insn_and_split like this: > (define_insn_and_split "*addsi_h8300" > [(set (match_operand:SI 0 "register_operand" "=r,r") > (plus:SI (match_operand:SI 1 "register_operand" "%0,0") > (match_operand:SI 2 "h8300_src_operand" "n,r")))] > "TARGET_H8300" > "#" > "reload_completed" > [(parallel [(set (match_dup 0) (plus:SI (match_dup 1) (match_dup 2))) > (clobber (reg:CC CC_REG))])]) We added the "#" which indicates the pattern has to be split and will never generate assembly code in and of itself. The "reload_completed" indicates when the spliter should fire (after reload). The subsequent RTL is how the pattern should be rewritten after reload (adds the clobber). Then you need a new pattern which matches the output of the splitter with the attached clobber. Something like this: > (define_insn "*addsi_h8300_clobber_flags" > [(set (match_operand:SI 0 "register_operand" "=r,r") > (plus:SI (match_operand:SI 1 "register_operand" "%0,0") > (match_operand:SI 2 "h8300_src_operand" "n,r"))) > (clobber (reg:CC CC_REG))] > "TARGET_H8300" > { > return output_plussi (operands); > } This is the form that we'll use after reload. It explicitly clobbers the condition codes which is the key here. Some have claimed this might be easier with define_subst, but I wandered it a bit and it didn't seem necessarily simpler in the end. But maybe someone with more experience with define_subst could make the transition simpler. Anyway, that has to be done for each and every pattern that modifies the condition codes. It's a ton of mechanical work. The m68k has the additional complication that condition codes can be either in cc0 or fcc0. So that has to be taken into account as well. You also have to rewrite the tst/cmp and conditional branching patterns to use the new style. Sadly until you do *all* of that you don't have anything you can really test. And even once you've done all that, the code quality is going to suffer because we're not doing any compare/tst elimination. To enable that you have to go back and start adding patterns like this: (define_insn "*addsi_h8300_set_flags" [(set (reg:CCNZ CC_REGNUM) (compare CCNZ (plus:SI (match_operand:SI 1 "register_operand" "%0,0") (match_operand:SI 2 "register_operand" "n, r")))) (set (match_operand:SI 0 "register_operand" "=r,r") (plus:SI (match_dup 1) (match_dup 2)))] "reload_completed && TARGET_H8300" { return output_plussi (operands); } This is where we describe (via the mode of the condition code register) what condition codes are set and how to extract them. These patterns can be added incrementally. But again, it's going to be a ton of work. My son is definitely still interested. But I doubt he'd likely be able to take up the task until the spring/summer of 2020. Additionally, there's some question of whether or not him doing the work could be viewed as a conflict of interest WRT the bug bounty because of his relationship to me. jeff
On Tue, Oct 29, 2019 at 8:34 PM Jeff Law <law@redhat.com> wrote: > > On 10/29/19 6:26 AM, John Paul Adrian Glaubitz wrote: > > Hello! > > > > We have raised $5000 to support anyone willing to work on this for the > > m68k target [1]. We really need the m68k to stay as it's essential to > > be able to compile for Linux/m68k, NetBSD/m68k and AROS/m68k (a new > > and ABI-compatible AmigaOS clone). I'm sure other communities like in the > > NeXTStep, Atari and Mac/m68k forums are interested in keeping the backend > > as well. I'm confident we will be able to raise even more money as the > > community is large and active. > > > >> If someone is interested in possibly doing a conversion, I would > >> strongly recommend reviewing the best documentation we have on the > >> subject: > >> > >> https://gcc.gnu.org/wiki/CC0Transition > >> > >> I worked with my son about a year ago to transition the v850 to MODE_CC > >> using that page as a guideline. We also have a partial transition for > >> the H8/300 port (not far enough to even build anything yet). I had > >> hoped we would finish the H8/300 port last year and would have tackled > >> another, but events have gotten in the way. > > > > Any chance that the unfinished code can be shared? I'm looking for any > > blueprints that can be used as a guidance for the work on the m68k > > backend. > I'm not sure it'd be all that useful. The v850 bits would be a better > starting point. The partial H8 transition isn't committed anywhere > because it would certainly break the port. > > > > > I have also created a guide on how to set up a QEMU virtual machine running > > a fully fledged Debian/m68k which can be used for the development work [2]. > > > >> The transition isn't terribly hard, but does take time because of the > >> number of patterns that have to be changed. > > > > I have already looked at the conversion done on the V850 backend [3] but so far > > I don't understand enough of the register representation stuff to be able > > to know where to start. I'm seeing that all the "cc" attributes are being removed > > but not replaced? > Right. The cc attributes were used to describe which condition codes > were valid after each insn. That information now has to be carried by > the mode of the condition code register. > > I'll walk you through one example from the H8 port. This isn't complete > enough to even test that it builds. > > > > (define_insn "*addsi_h8300" > > [(set (match_operand:SI 0 "register_operand" "=r,r") > > (plus:SI (match_operand:SI 1 "register_operand" "%0,0") > > (match_operand:SI 2 "h8300_src_operand" "n,r")))] > > "TARGET_H8300" > > { > > return output_plussi (operands); > > } > > [(set (attr "length") > > (symbol_ref "compute_plussi_length (operands)")) > > (set (attr "cc") > > (symbol_ref "compute_plussi_cc (operands)"))]) > > > That needs to turn into a define_insn_and_split. The idea is the form > above is what's seen prior to register allocation and reloading. After > reloading we turn it into a form with an attached clobber. We achieve > this by rewriting the pattern into a define_insn_and_split like this: > > > (define_insn_and_split "*addsi_h8300" > > [(set (match_operand:SI 0 "register_operand" "=r,r") > > (plus:SI (match_operand:SI 1 "register_operand" "%0,0") > > (match_operand:SI 2 "h8300_src_operand" "n,r")))] > > "TARGET_H8300" > > "#" > > "reload_completed" > > [(parallel [(set (match_dup 0) (plus:SI (match_dup 1) (match_dup 2))) > > (clobber (reg:CC CC_REG))])]) > > We added the "#" which indicates the pattern has to be split and will > never generate assembly code in and of itself. The "reload_completed" > indicates when the spliter should fire (after reload). The subsequent > RTL is how the pattern should be rewritten after reload (adds the clobber). > > Then you need a new pattern which matches the output of the splitter > with the attached clobber. Something like this: > > > (define_insn "*addsi_h8300_clobber_flags" > > [(set (match_operand:SI 0 "register_operand" "=r,r") > > (plus:SI (match_operand:SI 1 "register_operand" "%0,0") > > (match_operand:SI 2 "h8300_src_operand" "n,r"))) > > (clobber (reg:CC CC_REG))] > > "TARGET_H8300" > > { > > return output_plussi (operands); > > } > This is the form that we'll use after reload. It explicitly clobbers > the condition codes which is the key here. I think the wiki has better examples. That said, I wonder how much can be automated here, for example when just considering CCmode (m68k has setcc IIRC) then for example all define_insns like (define_insn "*cmpsi_cf" [(set (cc0) (compare (match_operand:SI 0 "nonimmediate_operand" "mrKs,r") (match_operand:SI 1 "general_operand" "r,mrKs")))] "TARGET_COLDFIRE" {...} could be rewritten based on seeing (set (cc0) ...) to a (set (reg:CC CC_REG) ...) plus adding a "&& reload_completed" condition. That could be done transparently by the .md file reader even? The various expanders need to be handled manually I guess, though somehow automagically generating the define_insn_and_split might be possible as well, no? That is, the work is quite repetitive (looking at m68k.md) and for this reason error-prone. But a case#2 conversion could essentially be automated? That is, do you see it as impossible to implement a case#2 cc0 "conversion" in gen*? > Some have claimed this might be easier with define_subst, but I wandered > it a bit and it didn't seem necessarily simpler in the end. But maybe > someone with more experience with define_subst could make the transition > simpler. > > Anyway, that has to be done for each and every pattern that modifies the > condition codes. It's a ton of mechanical work. The m68k has the > additional complication that condition codes can be either in cc0 or > fcc0. So that has to be taken into account as well. I don't see that it does this currently though (cc0 doesn't allow this). > You also have to rewrite the tst/cmp and conditional branching patterns > to use the new style. > > Sadly until you do *all* of that you don't have anything you can really > test. And even once you've done all that, the code quality is going to > suffer because we're not doing any compare/tst elimination. To enable > that you have to go back and start adding patterns like this: For m68k what helps is that you could concentrate on TARGET_COLDFIRE (or !TARGET_COLDFIRE). Maybe even split m68k.md along this. A lot of the define_insns also look like optimization (so could be disabled for the moment). > (define_insn "*addsi_h8300_set_flags" > [(set (reg:CCNZ CC_REGNUM) > (compare CCNZ (plus:SI (match_operand:SI 1 "register_operand" > "%0,0") > (match_operand:SI 2 "register_operand" > "n, r")))) > (set (match_operand:SI 0 "register_operand" "=r,r") > (plus:SI (match_dup 1) (match_dup 2)))] > "reload_completed && TARGET_H8300" > { > return output_plussi (operands); > } > > This is where we describe (via the mode of the condition code register) > what condition codes are set and how to extract them. These patterns > can be added incrementally. But again, it's going to be a ton of work. > > My son is definitely still interested. But I doubt he'd likely be able > to take up the task until the spring/summer of 2020. Additionally, > there's some question of whether or not him doing the work could be > viewed as a conflict of interest WRT the bug bounty because of his > relationship to me. > > > jeff >
Hi Jeff!! On 10/29/19 8:34 PM, Jeff Law wrote: >> Any chance that the unfinished code can be shared? I'm looking for any >> blueprints that can be used as a guidance for the work on the m68k >> backend. > I'm not sure it'd be all that useful. The v850 bits would be a better > starting point. The partial H8 transition isn't committed anywhere > because it would certainly break the port. I confused the H8 backend with the V850 backend, sorry for that. I have realized later that the V850 backend is the one where the conversion has been completed. > I'll walk you through one example from the H8 port. This isn't complete > enough to even test that it builds. (...) > This is where we describe (via the mode of the condition code register) > what condition codes are set and how to extract them. These patterns > can be added incrementally. But again, it's going to be a ton of work. Thanks for the detailed explanation. I will be going through this over the weekend trying to understand the details. > My son is definitely still interested. But I doubt he'd likely be able > to take up the task until the spring/summer of 2020. Additionally, > there's some question of whether or not him doing the work could be > viewed as a conflict of interest WRT the bug bounty because of his > relationship to me. I don't think there is a conflict of interest in any way. It's perfectly fine to pick up the task yourself and receive the bounty. The community doesn't really care who gets the job done. And since I don't assume your employer is paying you to work on the m68k backend, it's absolutely okay in my opinion to be paid for the task, time isn't free, especially developer time. It's very valuable to everyone in the various m68k communities to have an up-to-date version of GCC available which is why so many people have donated for the cause. Thanks a lot for your time and input! Adrian
Richard Biener <richard.guenther@gmail.com> writes: > On Tue, Oct 29, 2019 at 8:34 PM Jeff Law <law@redhat.com> wrote: >> >> On 10/29/19 6:26 AM, John Paul Adrian Glaubitz wrote: >> > Hello! >> > >> > We have raised $5000 to support anyone willing to work on this for the >> > m68k target [1]. We really need the m68k to stay as it's essential to >> > be able to compile for Linux/m68k, NetBSD/m68k and AROS/m68k (a new >> > and ABI-compatible AmigaOS clone). I'm sure other communities like in the >> > NeXTStep, Atari and Mac/m68k forums are interested in keeping the backend >> > as well. I'm confident we will be able to raise even more money as the >> > community is large and active. >> > >> >> If someone is interested in possibly doing a conversion, I would >> >> strongly recommend reviewing the best documentation we have on the >> >> subject: >> >> >> >> https://gcc.gnu.org/wiki/CC0Transition >> >> >> >> I worked with my son about a year ago to transition the v850 to MODE_CC >> >> using that page as a guideline. We also have a partial transition for >> >> the H8/300 port (not far enough to even build anything yet). I had >> >> hoped we would finish the H8/300 port last year and would have tackled >> >> another, but events have gotten in the way. >> > >> > Any chance that the unfinished code can be shared? I'm looking for any >> > blueprints that can be used as a guidance for the work on the m68k >> > backend. >> I'm not sure it'd be all that useful. The v850 bits would be a better >> starting point. The partial H8 transition isn't committed anywhere >> because it would certainly break the port. >> >> > >> > I have also created a guide on how to set up a QEMU virtual machine running >> > a fully fledged Debian/m68k which can be used for the development work [2]. >> > >> >> The transition isn't terribly hard, but does take time because of the >> >> number of patterns that have to be changed. >> > >> > I have already looked at the conversion done on the V850 backend [3] but so far >> > I don't understand enough of the register representation stuff to be able >> > to know where to start. I'm seeing that all the "cc" attributes are being removed >> > but not replaced? >> Right. The cc attributes were used to describe which condition codes >> were valid after each insn. That information now has to be carried by >> the mode of the condition code register. >> >> I'll walk you through one example from the H8 port. This isn't complete >> enough to even test that it builds. >> >> >> > (define_insn "*addsi_h8300" >> > [(set (match_operand:SI 0 "register_operand" "=r,r") >> > (plus:SI (match_operand:SI 1 "register_operand" "%0,0") >> > (match_operand:SI 2 "h8300_src_operand" "n,r")))] >> > "TARGET_H8300" >> > { >> > return output_plussi (operands); >> > } >> > [(set (attr "length") >> > (symbol_ref "compute_plussi_length (operands)")) >> > (set (attr "cc") >> > (symbol_ref "compute_plussi_cc (operands)"))]) >> >> >> That needs to turn into a define_insn_and_split. The idea is the form >> above is what's seen prior to register allocation and reloading. After >> reloading we turn it into a form with an attached clobber. We achieve >> this by rewriting the pattern into a define_insn_and_split like this: >> >> > (define_insn_and_split "*addsi_h8300" >> > [(set (match_operand:SI 0 "register_operand" "=r,r") >> > (plus:SI (match_operand:SI 1 "register_operand" "%0,0") >> > (match_operand:SI 2 "h8300_src_operand" "n,r")))] >> > "TARGET_H8300" >> > "#" >> > "reload_completed" >> > [(parallel [(set (match_dup 0) (plus:SI (match_dup 1) (match_dup 2))) >> > (clobber (reg:CC CC_REG))])]) >> >> We added the "#" which indicates the pattern has to be split and will >> never generate assembly code in and of itself. The "reload_completed" >> indicates when the spliter should fire (after reload). The subsequent >> RTL is how the pattern should be rewritten after reload (adds the clobber). >> >> Then you need a new pattern which matches the output of the splitter >> with the attached clobber. Something like this: >> >> > (define_insn "*addsi_h8300_clobber_flags" >> > [(set (match_operand:SI 0 "register_operand" "=r,r") >> > (plus:SI (match_operand:SI 1 "register_operand" "%0,0") >> > (match_operand:SI 2 "h8300_src_operand" "n,r"))) >> > (clobber (reg:CC CC_REG))] >> > "TARGET_H8300" >> > { >> > return output_plussi (operands); >> > } >> This is the form that we'll use after reload. It explicitly clobbers >> the condition codes which is the key here. > > I think the wiki has better examples. That said, I wonder how much can > be automated here, for example when just considering CCmode (m68k has > setcc IIRC) then for example all define_insns like > > (define_insn "*cmpsi_cf" > [(set (cc0) > (compare (match_operand:SI 0 "nonimmediate_operand" "mrKs,r") > (match_operand:SI 1 "general_operand" "r,mrKs")))] > "TARGET_COLDFIRE" > {...} > > could be rewritten based on seeing (set (cc0) ...) to a (set (reg:CC > CC_REG) ...) > plus adding a "&& reload_completed" condition. That could be done > transparently by the .md file reader even? > > The various expanders need to be handled manually I guess, though somehow > automagically generating the define_insn_and_split might be possible > as well, no? > > That is, the work is quite repetitive (looking at m68k.md) and for this reason > error-prone. But a case#2 conversion could essentially be automated? > > That is, do you see it as impossible to implement a case#2 cc0 "conversion" > in gen*? I don't think we should do an automatic conversion in gen*, since presumably that means that (cc0) sticks around in the .md files. It would also make it harder to do follow-on work if what the .md file says isn't what you actually get. So yeah, some parts of the change might be automatable, but I think they should be done using separate run-once scripts, with the result of the conversion checked in as the new source. Some manual work will still be needed on top. FWIW it's already possible to do the transform you mention with: s/(cc0)/(reg:CC CC_REGNUM_RC)/g (define_int_iterator CC_REGNUM_RC [(CC_REGNUM "reload_completed")]) But that's the easy bit to do manually or to script, so I don't think it really helps. And IMO adding explicit reload_completed checks would be better style. Thanks, Richard
Richard Sandiford <richard.sandiford@arm.com> wrote: > FWIW it's already possible to do the transform you mention with: > > s/(cc0)/(reg:CC CC_REGNUM_RC)/g > > (define_int_iterator CC_REGNUM_RC [(CC_REGNUM "reload_completed")]) Since "reload_completed" is referenced, this is only about the CC0 conversion, right? Switching to LRA is not required for this step. Regards, Gunther
On 10/30/19 11:52 AM, Gunther Nikl wrote: > Richard Sandiford <richard.sandiford@arm.com> wrote: >> FWIW it's already possible to do the transform you mention with: >> >> s/(cc0)/(reg:CC CC_REGNUM_RC)/g >> >> (define_int_iterator CC_REGNUM_RC [(CC_REGNUM "reload_completed")]) > > Since "reload_completed" is referenced, this is only about the CC0 > conversion, right? Switching to LRA is not required for this step. Correct jeff
On 10/30/19 6:52 PM, Gunther Nikl wrote: > Richard Sandiford <richard.sandiford@arm.com> wrote: >> FWIW it's already possible to do the transform you mention with: >> >> s/(cc0)/(reg:CC CC_REGNUM_RC)/g >> >> (define_int_iterator CC_REGNUM_RC [(CC_REGNUM "reload_completed")]) > > Since "reload_completed" is referenced, this is only about the CC0 > conversion, right? Switching to LRA is not required for this step. I think it would be nice though that anyone who does the cc0 transition would also switch over to LRA unless that would be too much of a burden. Adrian
On 10/30/19 11:57 AM, John Paul Adrian Glaubitz wrote: > On 10/30/19 6:52 PM, Gunther Nikl wrote: >> Richard Sandiford <richard.sandiford@arm.com> wrote: >>> FWIW it's already possible to do the transform you mention with: >>> >>> s/(cc0)/(reg:CC CC_REGNUM_RC)/g >>> >>> (define_int_iterator CC_REGNUM_RC [(CC_REGNUM "reload_completed")]) >> >> Since "reload_completed" is referenced, this is only about the CC0 >> conversion, right? Switching to LRA is not required for this step. > > I think it would be nice though that anyone who does the cc0 transition > would also switch over to LRA unless that would be too much of a burden. True, but the priority is the cc0 transition. Jeff
On 10/30/19 2:12 AM, Richard Biener wrote: > On Tue, Oct 29, 2019 at 8:34 PM Jeff Law <law@redhat.com> wrote: > > I think the wiki has better examples. That said, I wonder how much can > be automated here, for example when just considering CCmode (m68k has > setcc IIRC) then for example all define_insns like > > (define_insn "*cmpsi_cf" > [(set (cc0) > (compare (match_operand:SI 0 "nonimmediate_operand" "mrKs,r") > (match_operand:SI 1 "general_operand" "r,mrKs")))] > "TARGET_COLDFIRE" > {...} The compare/test insns are relatively straightforward as they're a fairly simple substitution. It's all the other insns that implicitly set cc0 that are so painful. > > The various expanders need to be handled manually I guess, though somehow > automagically generating the define_insn_and_split might be possible > as well, no? I was looking at define_subst to help here, but it didn't look like it was really going to simplify things in any significant way. Maybe someone with more experience with define_subst would see something that would dramtically simplify this process. > > That is, the work is quite repetitive (looking at m68k.md) and for this reason > error-prone. But a case#2 conversion could essentially be automated? It's a case2. ANd yes, it's highly repetitive. That's the part of the v850 conversion my son did. Once he had all the mechanical stuff in place I started banging it into shape. > > That is, do you see it as impossible to implement a case#2 cc0 "conversion" > in gen*? I'd be surprised if it could be that simple. > >> Some have claimed this might be easier with define_subst, but I wandered >> it a bit and it didn't seem necessarily simpler in the end. But maybe >> someone with more experience with define_subst could make the transition >> simpler. >> >> Anyway, that has to be done for each and every pattern that modifies the >> condition codes. It's a ton of mechanical work. The m68k has the >> additional complication that condition codes can be either in cc0 or >> fcc0. So that has to be taken into account as well. > > I don't see that it does this currently though (cc0 doesn't allow this). You're right. fcmp seems to just set the normal condition codes, just using a different insn. So the FP unit shouldn't be a major problem. > >> You also have to rewrite the tst/cmp and conditional branching patterns >> to use the new style. >> >> Sadly until you do *all* of that you don't have anything you can really >> test. And even once you've done all that, the code quality is going to >> suffer because we're not doing any compare/tst elimination. To enable >> that you have to go back and start adding patterns like this: > > For m68k what helps is that you could concentrate on TARGET_COLDFIRE > (or !TARGET_COLDFIRE). Maybe even split m68k.md along this. > > A lot of the define_insns also look like optimization (so could be disabled > for the moment). That's certainly how I'd approach it. Disable everything not strictly needed, convert what has to be converted, then bring back the patterns incrementally. Jeff
> On Oct 30, 2019, at 2:24 PM, Jeff Law <law@redhat.com> wrote: > > On 10/30/19 2:12 AM, Richard Biener wrote: >> On Tue, Oct 29, 2019 at 8:34 PM Jeff Law <law@redhat.com> wrote: > >> >> I think the wiki has better examples. That said, I wonder how much can >> be automated here, for example when just considering CCmode (m68k has >> setcc IIRC) then for example all define_insns like >> >> (define_insn "*cmpsi_cf" >> [(set (cc0) >> (compare (match_operand:SI 0 "nonimmediate_operand" "mrKs,r") >> (match_operand:SI 1 "general_operand" "r,mrKs")))] >> "TARGET_COLDFIRE" >> {...} > The compare/test insns are relatively straightforward as they're a > fairly simple substitution. It's all the other insns that implicitly > set cc0 that are so painful. > > >> >> The various expanders need to be handled manually I guess, though somehow >> automagically generating the define_insn_and_split might be possible >> as well, no? > I was looking at define_subst to help here, but it didn't look like it > was really going to simplify things in any significant way. Maybe > someone with more experience with define_subst would see something that > would dramtically simplify this process. I've found define_subst to be quite helpful for generating the pairs of "clobbers CC" and "sets CC in a useful way from the operation result" pairs that Richard was referring to. pdp11.md shows what I learned; it may be applicable elsewhere too. This is for a "case 2" target, one that modifies CC on most operations (essentially all those that do arithmetic). paul
Hi! On Tue, Oct 29, 2019 at 01:34:15PM -0600, Jeff Law wrote: > That needs to turn into a define_insn_and_split. The idea is the form > above is what's seen prior to register allocation and reloading. After > reloading we turn it into a form with an attached clobber. We achieve > this by rewriting the pattern into a define_insn_and_split like this: > > > (define_insn_and_split "*addsi_h8300" > > [(set (match_operand:SI 0 "register_operand" "=r,r") > > (plus:SI (match_operand:SI 1 "register_operand" "%0,0") > > (match_operand:SI 2 "h8300_src_operand" "n,r")))] > > "TARGET_H8300" > > "#" > > "reload_completed" > > [(parallel [(set (match_dup 0) (plus:SI (match_dup 1) (match_dup 2))) > > (clobber (reg:CC CC_REG))])]) > > We added the "#" which indicates the pattern has to be split and will > never generate assembly code in and of itself. It is important to note that this indication is mostly a comment, and nothing more. The split condition ("reload_completed", here) is the only thing that determines if something is split or not. The exception is that final.c *does* look if the template is "#", and splits while outputting to asm. Normally though everything is split earlier already (there is a split pass almost immediately before final, one of the two that is run at -O0 as well). It's great to always write "#" here, it is nice documentation ("this insn is always split"), but you still need to get your split condition correct. Segher
diff --git a/gcc/config.gcc b/gcc/config.gcc index 69d0a024d85..0c1637e8be1 100644 --- a/gcc/config.gcc +++ b/gcc/config.gcc @@ -248,6 +248,12 @@ md_file= # Obsolete configurations. case ${target} in tile*-*-* \ + avr*-*-* \ + h8300*-*-* \ + cris*-*-* \ + m68k*-*-* \ + vax*-*-* \ + cr16*-*-* \ ) if test "x$enable_obsolete" != xyes; then echo "*** Configuration ${target} is obsolete." >&2 @@ -273,7 +279,6 @@ case ${target} in | arm*-*-uclinux* \ | i[34567]86-go32-* \ | i[34567]86-*-go32* \ - | m68k-*-uclinuxoldabi* \ | mips64orion*-*-rtems* \ | pdp11-*-bsd \ | powerpc*-*-linux*paired* \ @@ -294,7 +299,6 @@ case ${target} in | *-*-solaris2.[0-9].* \ | *-*-solaris2.10* \ | *-*-sysv* \ - | vax-*-vms* \ ) echo "*** Configuration ${target} not supported" 1>&2 exit 1