Re: [PATCH, FT32] initial support
On 05/28/2015 03:50 PM, Eric Botcazou wrote: Thanks very much. ChangeLog entry: 2015-05-14 James Bowman * configure.ac: FT32 target added * libgcc/config.host: FT32 target added * gcc/config/ft32/: FT32 target added * libgcc/config/ft32/: FT32 target added * gcc/doc/install.texi, invoke.texi, md.texi: FT32 details added * gcc/doc/contrib.texi: self added * contrib/config-list.mk: FT32 target added * configure: Regenerated That's wrong, you cannot add just a single entry to the toplevel ChangeLog, you need to add entries to the ChangeLog of every subdirectory instead: gcc, libgcc, contrib, etc. Please fix. Done. jeff
Re: [PATCH, FT32] initial support
> Thanks very much. ChangeLog entry: > > 2015-05-14 James Bowman > > * configure.ac: FT32 target added > * libgcc/config.host: FT32 target added > * gcc/config/ft32/: FT32 target added > * libgcc/config/ft32/: FT32 target added > * gcc/doc/install.texi, invoke.texi, md.texi: FT32 details added > * gcc/doc/contrib.texi: self added > * contrib/config-list.mk: FT32 target added > * configure: Regenerated That's wrong, you cannot add just a single entry to the toplevel ChangeLog, you need to add entries to the ChangeLog of every subdirectory instead: gcc, libgcc, contrib, etc. Please fix. -- Eric Botcazou
Re: [PATCH, FT32] initial support
On 05/14/2015 06:43 PM, James Bowman wrote: On 05/14/2015 01:24 PM, Segher Boessenkool wrote: On Thu, May 14, 2015 at 12:31:40PM -0600, Jeff Law wrote: On 05/14/2015 11:36 AM, Segher Boessenkool wrote: The alternative that allows move to mem is alt 1, but it thinks the operand doesn't match (it is "B" or "W"), it picks alt 0, loop, everyone unhappy. "B" should match it seems? (Why does IRA think r56 should be in memory? Yeah, good question.) Independent of that, if a reg-reg move generated by LRA (which is really a mem->reg or reg->mem) blows up on this target for some reason, then that needs to be addressed independently of whether or not IRA might have made a bad choice for which pseudo to spill. Yes. It probably is because of the "volatile" I inserted for it to fail at all, anyway. Changing constraints ABWef to be define_memory_constraint (instead of define_constraint) makes this testcase pass. It also makes the build pass (it failed before; 90 reloads in libgcc). Pretty miserable code for those memory accesses, but hey, better than an ICE ;-) Makes sense if it wasn't defined as a memory constraint that LRA would be having trouble -- odds are reload would have barf'd at some point too. The only difference is I'm actually still better at understanding what reload's doing from its dumps than LRA. It's hard to throw away decades of retained knowledge earned the hard way. James, can you go ahead and make that change to ABWef and enable LRA and commit your changes to the trunk? OK, ABWef changes made, and fixed a couple of pattern holes that they exposed. Added a new target option "-mlra" to enable LRA. Updated invoking.texi. Please can someone commit this, if appropriate, as I do not have write access to the tree. Thanks very much. ChangeLog entry: 2015-05-14 James Bowman * configure.ac: FT32 target added * libgcc/config.host: FT32 target added * gcc/config/ft32/: FT32 target added * libgcc/config/ft32/: FT32 target added * gcc/doc/install.texi, invoke.texi, md.texi: FT32 details added * gcc/doc/contrib.texi: self added * contrib/config-list.mk: FT32 target added * configure: Regenerated Well, the plan will be for you to maintain the port. So you should go ahead and get write access so that you can commit changes to the port. See authenticated access on this page https://gcc.gnu.org/svnwrite.html List me as your sponsor Thanks for your patience, Jeff
RE: [PATCH, FT32] initial support
> On 05/14/2015 01:24 PM, Segher Boessenkool wrote: > > On Thu, May 14, 2015 at 12:31:40PM -0600, Jeff Law wrote: > >> On 05/14/2015 11:36 AM, Segher Boessenkool wrote: > >>> The alternative that allows move to mem is alt 1, but it thinks the > >>> operand > >>> doesn't match (it is "B" or "W"), it picks alt 0, loop, everyone unhappy. > >>> > >>> "B" should match it seems? > >>> > >>> (Why does IRA think r56 should be in memory? Yeah, good question.) > >> Independent of that, if a reg-reg move generated by LRA (which is really > >> a mem->reg or reg->mem) blows up on this target for some reason, then > >> that needs to be addressed independently of whether or not IRA might > >> have made a bad choice for which pseudo to spill. > > > > Yes. It probably is because of the "volatile" I inserted for it > > to fail at all, anyway. > > > > Changing constraints ABWef to be define_memory_constraint (instead of > > define_constraint) makes this testcase pass. It also makes the build > > pass (it failed before; 90 reloads in libgcc). > > > > Pretty miserable code for those memory accesses, but hey, better > > than an ICE ;-) > Makes sense if it wasn't defined as a memory constraint that LRA would > be having trouble -- odds are reload would have barf'd at some point too. > > The only difference is I'm actually still better at understanding what > reload's doing from its dumps than LRA. It's hard to throw away decades > of retained knowledge earned the hard way. > > James, can you go ahead and make that change to ABWef and enable LRA and > commit your changes to the trunk? OK, ABWef changes made, and fixed a couple of pattern holes that they exposed. Added a new target option "-mlra" to enable LRA. Updated invoking.texi. Please can someone commit this, if appropriate, as I do not have write access to the tree. Thanks very much. ChangeLog entry: 2015-05-14 James Bowman * configure.ac: FT32 target added * libgcc/config.host: FT32 target added * gcc/config/ft32/: FT32 target added * libgcc/config/ft32/: FT32 target added * gcc/doc/install.texi, invoke.texi, md.texi: FT32 details added * gcc/doc/contrib.texi: self added * contrib/config-list.mk: FT32 target added * configure: Regenerated -- James Bowman FTDI Open Source Liaison gcc-ft32.txt.gz Description: gcc-ft32.txt.gz
Re: [PATCH, FT32] initial support
On 05/14/2015 01:24 PM, Segher Boessenkool wrote: On Thu, May 14, 2015 at 12:31:40PM -0600, Jeff Law wrote: On 05/14/2015 11:36 AM, Segher Boessenkool wrote: The alternative that allows move to mem is alt 1, but it thinks the operand doesn't match (it is "B" or "W"), it picks alt 0, loop, everyone unhappy. "B" should match it seems? (Why does IRA think r56 should be in memory? Yeah, good question.) Independent of that, if a reg-reg move generated by LRA (which is really a mem->reg or reg->mem) blows up on this target for some reason, then that needs to be addressed independently of whether or not IRA might have made a bad choice for which pseudo to spill. Yes. It probably is because of the "volatile" I inserted for it to fail at all, anyway. Changing constraints ABWef to be define_memory_constraint (instead of define_constraint) makes this testcase pass. It also makes the build pass (it failed before; 90 reloads in libgcc). Pretty miserable code for those memory accesses, but hey, better than an ICE ;-) Makes sense if it wasn't defined as a memory constraint that LRA would be having trouble -- odds are reload would have barf'd at some point too. The only difference is I'm actually still better at understanding what reload's doing from its dumps than LRA. It's hard to throw away decades of retained knowledge earned the hard way. James, can you go ahead and make that change to ABWef and enable LRA and commit your changes to the trunk? Thanks, jeff
Re: [PATCH, FT32] initial support
On Thu, May 14, 2015 at 12:31:40PM -0600, Jeff Law wrote: > On 05/14/2015 11:36 AM, Segher Boessenkool wrote: > >The alternative that allows move to mem is alt 1, but it thinks the operand > >doesn't match (it is "B" or "W"), it picks alt 0, loop, everyone unhappy. > > > >"B" should match it seems? > > > >(Why does IRA think r56 should be in memory? Yeah, good question.) > Independent of that, if a reg-reg move generated by LRA (which is really > a mem->reg or reg->mem) blows up on this target for some reason, then > that needs to be addressed independently of whether or not IRA might > have made a bad choice for which pseudo to spill. Yes. It probably is because of the "volatile" I inserted for it to fail at all, anyway. Changing constraints ABWef to be define_memory_constraint (instead of define_constraint) makes this testcase pass. It also makes the build pass (it failed before; 90 reloads in libgcc). Pretty miserable code for those memory accesses, but hey, better than an ICE ;-) Segher
Re: [PATCH, FT32] initial support
On 05/14/2015 11:36 AM, Segher Boessenkool wrote: The alternative that allows move to mem is alt 1, but it thinks the operand doesn't match (it is "B" or "W"), it picks alt 0, loop, everyone unhappy. "B" should match it seems? (Why does IRA think r56 should be in memory? Yeah, good question.) Independent of that, if a reg-reg move generated by LRA (which is really a mem->reg or reg->mem) blows up on this target for some reason, then that needs to be addressed independently of whether or not IRA might have made a bad choice for which pseudo to spill. Jeff
Re: [PATCH, FT32] initial support
On 05/14/2015 09:20 AM, James Bowman wrote: On Thu, May 14, 2015 at 07:54:02AM -0500, Segher Boessenkool wrote: I cannot reproduce this with your testcase. Please post the *exact* testcase (nothing snipped) and the command line you used? Making the array volatile and using optimisation helped. Kaboom. It wants to reload pretty much everything. Looking at your code, it seems you use the ancient interfaces instead of TARGET_LEGITIMATE_ADDRESS_P, I don't think LRA works with that? The FT32 target uses TARGET_ADDR_SPACE_LEGITIMATE_ADDRESS_P (ft32.c line 856). The FT32 itself is a Harvard architecture, and the ft32 port uses address spaces to distinguish between program and data memory. Of the ports that currently support LRA (arc, mips, rs6000, s380, sh) none use address spaces. Perhaps this port's use of address spaces is causing the problem? I don't think so, it feels like something else to me. But I do have a larger concern that if LRA hasn't been used much, if at all, on targets that have multiple address spaces, then we may have other lurking issues. Anyway: In .ira we have: (insn 11 7 12 2 (set (reg:SI 51) (const_int -900 [0xfc7c])) j.c:7 25 {*movsi} (expr_list:REG_EQUIV (const_int -900 [0xfc7c]) (nil))) (insn 12 11 18 2 (set (reg/f:SI 50) (plus:SI (reg/f:SI 0 $fp) (reg:SI 51))) j.c:7 2 {addsi3} (expr_list:REG_DEAD (reg:SI 51) (expr_list:REG_EQUIV (plus:SI (reg/f:SI 0 $fp) (const_int -900 [0xfc7c])) (nil Where r51 will get a hard reg, but r50 will not. This is confirmed by looking at the coloring dump in .ira: Loop 0 (parent -1, header bb2, depth 0) bbs: 4 3 2 all: 0r45 1r54 2r50 3r51 4r48 5r44 10r52 11r43 modified regnos: 43 44 45 48 50 51 52 54 border: Pressure: GENERAL_REGS=6 Hard reg set forest: 0:( 2-29)@0 1:( 2-6 8-29)@87680 Spill a2(r50,l0) So r50 gets spilled to memory. That's going to cause insn 12 to have an output reload because it can't store into memory directly. Choosing alt 1 in insn 12: (0) r (1) r (2) r {addsi3} Creating newreg=55 from oldreg=50, assigning class GENERAL_REGS to r55 12: r55:SI=$fp:SI+r51:SI REG_DEAD r51:SI REG_EQUIV $fp:SI-0x384 Inserting insn reload after: 32: r50:SI=r55:SI So we're going to use r55 to hold the result of the arithmetic, then shove it into the stack slot allocated for r50. That all seems normal. But for some reason things blow up after that, it's almost as if LRA doesn't like the trival reg-reg copy emitted to handle the output reload. At this point I'd recommend committing the port as-is and attacking LRA as a follow-up. jeff
Re: [PATCH, FT32] initial support
On Thu, May 14, 2015 at 03:20:46PM +, James Bowman wrote: > > It wants to reload pretty much everything. Looking at your code, > > it seems you use the ancient interfaces instead of > > TARGET_LEGITIMATE_ADDRESS_P, I don't think LRA works with that? > > The FT32 target uses TARGET_ADDR_SPACE_LEGITIMATE_ADDRESS_P > (ft32.c line 856). Ah I see. > The FT32 itself is a Harvard architecture, and the ft32 port uses > address spaces to distinguish between program and data memory. > > Of the ports that currently support LRA (arc, mips, rs6000, s380, sh) none > use address spaces. > Perhaps this port's use of address spaces is causing the problem? That could certainly cause problems. I don't think it's likely it causes all this though. Adding some debug, it looks like LRA never calls the TARGET_ADDR_SPACE_LEGITIMATE_ADDRESS_P hook? Not during the spilling it does, anyway. Here is what it _does_ do: Choosing alt 1 in insn 12: (0) r (1) r (2) r {addsi3} Creating newreg=58 from oldreg=56, assigning class GENERAL_REGS to r58 12: r58:SI=$fp:SI+r55:SI REG_DEAD r55:SI REG_EQUIV $fp:SI-0x384 Inserting insn reload after: 32: r56:SI=r58:SI so it decides to use the "r" constraint everywhere in that addsi3 insn, thinks it isn't all okay yet (and it isn't, r56 was allocated to mem (see the ira dump), has no hard reg yet), and solves it by generating an extra move. That should be a move from the reg to mem, but it doesn't want to: 0 Non input pseudo reload: reject++ 1 Non pseudo reload: reject++ alt=0,overall=608,losers=1,rld_nregs=1 alt=1: Bad operand -- refuse 0 Non input pseudo reload: reject++ alt=2: Bad operand -- refuse 0 Non input pseudo reload: reject++ alt=3: Bad operand -- refuse 0 Non input pseudo reload: reject++ alt=4: Bad operand -- refuse 0 Non input pseudo reload: reject++ alt=5: Bad operand -- refuse alt=6: Bad operand -- refuse 0 Non input pseudo reload: reject++ alt=7: Bad operand -- refuse 0 Non input pseudo reload: reject++ alt=8: Bad operand -- refuse Choosing alt 0 in insn 32: (0) =r (1) r {*movsi} Creating newreg=59 from oldreg=56, assigning class GENERAL_REGS to r59 32: r59:SI=r58:SI Inserting insn reload after: 33: r56:SI=r59:SI The alternative that allows move to mem is alt 1, but it thinks the operand doesn't match (it is "B" or "W"), it picks alt 0, loop, everyone unhappy. "B" should match it seems? (Why does IRA think r56 should be in memory? Yeah, good question.) Segher
RE: [PATCH, FT32] initial support
> On Thu, May 14, 2015 at 07:54:02AM -0500, Segher Boessenkool wrote: > > I cannot reproduce this with your testcase. Please post the *exact* > > testcase (nothing snipped) and the command line you used? > > Making the array volatile and using optimisation helped. Kaboom. > > It wants to reload pretty much everything. Looking at your code, > it seems you use the ancient interfaces instead of > TARGET_LEGITIMATE_ADDRESS_P, I don't think LRA works with that? The FT32 target uses TARGET_ADDR_SPACE_LEGITIMATE_ADDRESS_P (ft32.c line 856). The FT32 itself is a Harvard architecture, and the ft32 port uses address spaces to distinguish between program and data memory. Of the ports that currently support LRA (arc, mips, rs6000, s380, sh) none use address spaces. Perhaps this port's use of address spaces is causing the problem? -- James Bowman FTDI Open Source Liaison
Re: [PATCH, FT32] initial support
On Thu, May 14, 2015 at 07:54:02AM -0500, Segher Boessenkool wrote: > I cannot reproduce this with your testcase. Please post the *exact* > testcase (nothing snipped) and the command line you used? Making the array volatile and using optimisation helped. Kaboom. It wants to reload pretty much everything. Looking at your code, it seems you use the ancient interfaces instead of TARGET_LEGITIMATE_ADDRESS_P, I don't think LRA works with that? Segher
Re: [PATCH, FT32] initial support
On Thu, May 14, 2015 at 01:44:48AM +, James Bowman wrote: > > On Tue, May 12, 2015 at 10:17:01PM +, James Bowman wrote: > > > It seems that with whenever a function's frame is bigger than 512 bytes, > > > LRA loops. I cannot reproduce this with your testcase. Please post the *exact* testcase (nothing snipped) and the command line you used? Segher
Re: [PATCH, FT32] initial support
On Thu, May 14, 2015 at 01:44:48AM +, James Bowman wrote: > > Compile with -da to get dump files, look at the .reload one (which is > > for LRA if that is enabled), stare long and hard. I recommend coffee. > > OK! Looking more at this, the LRA is not actually looping on the link, but on > an address calculation [snip] > And LRA loops on insn 37, repeatedly allocating a register for (reg:SI 97). Now your job is to figure out why it does that. It prints out for every innstruction what alternative it chose, and why it thinks that is a good plan or not. Since nothing works, it puts in some reload insns; it also shows those. But since in your case the reloads do not in fact solve anything, it loops. Since you have a frame pointer, was this a -O0 compile? [snip, working case:] > (insn 7 11 8 2 (set (reg:SI 2 $r0) > (plus:SI (reg/f:SI 0 $fp) > (reg:SI 43))) /home/james/tmp/x.c:31 2 {addsi3} > (expr_list:REG_DEAD (reg:SI 43) > (nil))) > > They look very similar. I am currently baffled. This doesn't need reloading the dest of the set, since it already has a hard register. > > > Do you think it would be easier to make the submission as is, then debug > > > the LRA issues from that point? If so, I have attached the current patch > > > set. > > > > You should add a -mlra option so other people can easily enable it, too; > > also handy later (when it defaults to on) when LRA blows up (you can > > workaround with -mno-lra then). > > Sounds good to me. Would that be an acceptable way to get the FT32 port > into the tree? I don't make those decisions. Having -mlra is of course not enough; but it can help you. And it's trivial to implement, so why are we talking about this :-) There are other things wrong with your port as well, so you'll take some time to fix those up anyway, and maybe the LRA issue isn't so hard at all. Having a -mlra option is nice in any case, as long as you haven't converted over to LRA fully. > I am very happy to go with the general flow towards LRA, > but at this point perhaps it may be a little early? I think LRA should work fine for you, I didn't see anything excitingly different from other archs in your port (but I didn't look in detail). Do you have test results? Segher
RE: [PATCH, FT32] initial support
> On Tue, May 12, 2015 at 10:17:01PM +, James Bowman wrote: > > It seems that with whenever a function's frame is bigger than 512 bytes, > > LRA loops. Likely this causes a problem because there is no actual > > instruction for subtracting constants more than 512. However, there is a > > "link" pattern that allows this. It is puzzling. > > That "link" pattern does > > (minus (reg) (imm)) > > but that is not canonical RTL; it should be written > > (plus (reg) (-imm)) > OK, thanks. I have corrected this. Seeing the same failure with LRA. > Compile with -da to get dump files, look at the .reload one (which is > for LRA if that is enabled), stare long and hard. I recommend coffee. OK! Looking more at this, the LRA is not actually looping on the link, but on an address calculation So this code: int runtest_bigframe() { unsigned int i; unsigned char buf[900]; for (i = 0; i < 900; i++) buf[i] = (i % 237); Causes this address calculation, attempting to compute the address of buf[]: (insn 36 30 37 2 (set (reg:SI 96) (const_int -900 [0xfc7c])) /home/james/tmp/x.c:11 25 {*movsi} (expr_list:REG_EQUIV (const_int -900 [0xfc7c]) (nil))) (insn 37 36 39 2 (set (reg:SI 97) (plus:SI (reg/f:SI 0 $fp) (reg:SI 96))) /home/james/tmp/x.c:11 2 {addsi3} (expr_list:REG_DEAD (reg:SI 96) (expr_list:REG_EQUIV (plus:SI (reg/f:SI 0 $fp) (const_int -900 [0xfc7c])) (nil And LRA loops on insn 37, repeatedly allocating a register for (reg:SI 97). However, something like this: void foo (void) { unsigned char buf[900]; bar(buf); } Happily compiles, even though the address calculation is identical! foo: link $fp,924 ldk.l $r0,-900 add.l $r0,$fp,$r0 call bar unlink $r29 return Here is the relevant part of the dump, just before reload as above: (insn 11 6 7 2 (set (reg:SI 43) (const_int -900 [0xfc7c])) /home/james/tmp/x.c:31 25 {*movsi} (expr_list:REG_EQUIV (const_int -900 [0xfc7c]) (nil))) (insn 7 11 8 2 (set (reg:SI 2 $r0) (plus:SI (reg/f:SI 0 $fp) (reg:SI 43))) /home/james/tmp/x.c:31 2 {addsi3} (expr_list:REG_DEAD (reg:SI 43) (nil))) They look very similar. I am currently baffled. > > Do you think it would be easier to make the submission as is, then debug > > the LRA issues from that point? If so, I have attached the current patch > > set. > > You should add a -mlra option so other people can easily enable it, too; > also handy later (when it defaults to on) when LRA blows up (you can > workaround with -mno-lra then). Sounds good to me. Would that be an acceptable way to get the FT32 port into the tree? I am very happy to go with the general flow towards LRA, but at this point perhaps it may be a little early? -- James Bowman FTDI Open Source Liaison
Re: [PATCH, FT32] initial support
On Tue, May 12, 2015 at 10:17:01PM +, James Bowman wrote: > It seems that with whenever a function's frame is bigger than 512 bytes, > LRA loops. Likely this causes a problem because there is no actual > instruction for subtracting constants more than 512. However, there is a > "link" pattern that allows this. It is puzzling. That "link" pattern does (minus (reg) (imm)) but that is not canonical RTL; it should be written (plus (reg) (-imm)) Compile with -da to get dump files, look at the .reload one (which is for LRA if that is enabled), stare long and hard. I recommend coffee. > Do you think it would be easier to make the submission as is, then debug > the LRA issues from that point? If so, I have attached the current patch set. You should add a -mlra option so other people can easily enable it, too; also handy later (when it defaults to on) when LRA blows up (you can workaround with -mno-lra then). Segher
Re: [PATCH, FT32] initial support
On May 12, 2015, at 3:36 PM, Jeff Law wrote: > > It really depends on the complexity of getting LRA working for the target and > given what I saw when looking at the port, I don't believe it should be much > work. LRA should default to on? Only preexisting ports about ask for, and get non-LRA compilers? :-)
Re: [PATCH, FT32] initial support
On 05/12/2015 04:17 PM, James Bowman wrote: On 05/08/2015 06:04 PM, James Bowman wrote: Are you using LRA or reload? The former is definitely preferred and for a simple target like this, I'd expect the transition to be very easy. I'm using reload. Attempting to naively switch on LRA resulted in internal compiler error: Max. number of generated reload insns per insn is achieved (90) so I would be very interested in a pointer an LRA clues.D Thanks. Vladimir Makarov would be the best contact point. I'm a bit surprised you ran into this issue given the simplicity of the port. I'd minimize the testcase, then enable debugging dumps, then look at the insn that's causing LRA to loop. If Vlad helps, he's probably going to want that minimized testcase as well, so it's time well spent. It seems that with whenever a function's frame is bigger than 512 bytes, LRA loops. Likely this causes a problem because there is no actual instruction for subtracting constants more than 512. However, there is a "link" pattern that allows this. It is puzzling. Is the subtraction part of the prologue/epilogue or some address calculation elsewhere in the code? What do you do prior to reload for subtracting a large constant? Presumably you copy it into a register then do a reg-reg subtraction? You might start by setting a breakpoint in lra_emit_add and see what kinds of insns its emits. If you're getting in there, I'd hazard a guess that it's generating reload insns that themselves need reloads. Do you think it would be easier to make the submission as is, then debug the LRA issues from that point? If so, I have attached the current patch set. Not sure yet :-) We really want to get away from the reload path and one way is to stop accepting ports that use it. We've done similar things in the past with other deprecated codepaths. It really depends on the complexity of getting LRA working for the target and given what I saw when looking at the port, I don't believe it should be much work. Thanks. -- James Bowman FTDI Open Source Liaison
RE: [PATCH, FT32] initial support
> On 05/08/2015 06:04 PM, James Bowman wrote: > > > >> Are you using LRA or reload? The former is definitely preferred and for > >> a simple target like this, I'd expect the transition to be very easy. > > > > I'm using reload. Attempting to naively switch on LRA resulted in > >internal compiler error: Max. number of generated reload insns per insn > > is achieved (90) > > so I would be very interested in a pointer an LRA clues.D Thanks. > Vladimir Makarov would be the best contact point. I'm a bit surprised > you ran into this issue given the simplicity of the port. > > I'd minimize the testcase, then enable debugging dumps, then look at the > insn that's causing LRA to loop. If Vlad helps, he's probably going to > want that minimized testcase as well, so it's time well spent. > It seems that with whenever a function's frame is bigger than 512 bytes, LRA loops. Likely this causes a problem because there is no actual instruction for subtracting constants more than 512. However, there is a "link" pattern that allows this. It is puzzling. Do you think it would be easier to make the submission as is, then debug the LRA issues from that point? If so, I have attached the current patch set. Thanks. -- James Bowman FTDI Open Source Liaison gcc-ft32.txt.gz Description: gcc-ft32.txt.gz
Re: [PATCH, FT32] initial support
On 05/08/2015 06:04 PM, James Bowman wrote: Are you using LRA or reload? The former is definitely preferred and for a simple target like this, I'd expect the transition to be very easy. I'm using reload. Attempting to naively switch on LRA resulted in internal compiler error: Max. number of generated reload insns per insn is achieved (90) so I would be very interested in a pointer an LRA clues.D Thanks. Vladimir Makarov would be the best contact point. I'm a bit surprised you ran into this issue given the simplicity of the port. I'd minimize the testcase, then enable debugging dumps, then look at the insn that's causing LRA to loop. If Vlad helps, he's probably going to want that minimized testcase as well, so it's time well spent. multidelta and other tools exist which can help with the minimization process. jeff
Re: [PATCH, FT32] initial support
On 03/19/2015 09:28 AM, James Bowman wrote: Second ping. Also, have attached updated patchset for the current gcc. Thanks. Thanks. I don't see anything particularly worrisome from a correctness standpoint. You may need to make minor updates to your .h file to cope with some of Trevor's work around eliminating conditionally compiled code. Obviously the sooner we wrap up any lingering details the better since Trevor's work might be a bit of a moving target. Similarly the work around changing the types of toplevel rtx structures to rtx_insn * from rtx may hit your .c and .md file in minor ways. That work is also ongoing so will likely be a moving target as well. However, from a quick scan of ft32.c, I don't see much code that would likely be affected by those changes. I didn't see any matching constraints in the md file, so you shouldn't be affected by the additional checking recently added to genrecog. The change to the MAINTAINERS file will happen after you're approved by the steering committee as a maintainer. For a new port like this it'd be silly to do anything other than have you as the maintainer, but we'll run through the usual procedure. I assume you've got a copyright assignment on file with the FSF for this work? I'm going to assume all the assembly code is correct :-) Some minor notes: You seem to have defined TRULY_NOOP_TRUNCATION twice. Are shift counts truncated on the ft32? You might want to define SHIFT_COUNT_TRUNCATED Are you using LRA or reload? The former is definitely preferred and for a simple target like this, I'd expect the transition to be very easy. ft32.c should be following the GNU style conventions. It's particularly bad in where it's placing the open/close braces. We do if (condition) { code } else { more code } But I see a lot of if (condition) { code } else { more code } Similarly you tend to have code like if (cond1 && cond2 && cond3) We write it as if (cond1 && cond2 && cond3) These seem minor, but a consistent convention really makes it easier when someone else has to fixup something in your port. You can use "indent" to fix this stuff up quickly. Jeff
RE: [PATCH, FT32] initial support
Second ping. Also, have attached updated patchset for the current gcc. Thanks. -- James Bowman FTDI Open Source Liaison From: Joseph Myers [jos...@codesourcery.com] Sent: Tuesday, February 17, 2015 2:06 AM To: James Bowman Cc: gcc-patches@gcc.gnu.org Subject: RE: [PATCH, FT32] initial support On Mon, 16 Feb 2015, James Bowman wrote: > I have updated the target options. Space-saving is now enabled by > -Os. There is also a new option -msim to enable building for the > simulator (the simulator is pending submission to gdb-binutils). The documentation in this patch doesn't seem to have been updated for those changes. -- Joseph S. Myers jos...@codesourcery.com gcc-ft32.txt.gz Description: gcc-ft32.txt.gz
RE: [PATCH, FT32] initial support
> On Mon, 16 Feb 2015, James Bowman wrote: > > > I have updated the target options. Space-saving is now enabled by > > -Os. There is also a new option -msim to enable building for the > > simulator (the simulator is pending submission to gdb-binutils). > > The documentation in this patch doesn't seem to have been updated for > those changes. > Ping. Also, have attached updated patchset for the current gcc. Thanks. -- James Bowman FTDI Open Source Liaison gcc-ft32.txt.gz Description: gcc-ft32.txt.gz
RE: [PATCH, FT32] initial support
Aha yes. Revised attached. invoke.texi now has: These options are defined specifically for the FT32 port. @table @gcctabopt @item -msim @opindex msim Specifies that the program will be run on the simulator. This causes an alternate runtime startup and library to be linked. You must not use this option when generating programs that will run on real hardware; you must provide your own runtime library for whatever I/O functions are needed. @end table -- James Bowman FTDI Open Source Liaison From: Joseph Myers [jos...@codesourcery.com] Sent: Tuesday, February 17, 2015 2:06 AM To: James Bowman Cc: gcc-patches@gcc.gnu.org Subject: RE: [PATCH, FT32] initial support On Mon, 16 Feb 2015, James Bowman wrote: > I have updated the target options. Space-saving is now enabled by > -Os. There is also a new option -msim to enable building for the > simulator (the simulator is pending submission to gdb-binutils). The documentation in this patch doesn't seem to have been updated for those changes. -- Joseph S. Myers jos...@codesourcery.com gcc-ft32.txt.gz Description: gcc-ft32.txt.gz
RE: [PATCH, FT32] initial support
On Mon, 16 Feb 2015, James Bowman wrote: > I have updated the target options. Space-saving is now enabled by > -Os. There is also a new option -msim to enable building for the > simulator (the simulator is pending submission to gdb-binutils). The documentation in this patch doesn't seem to have been updated for those changes. -- Joseph S. Myers jos...@codesourcery.com
RE: [PATCH, FT32] initial support
> From: Joseph Myers > ... > > +@table @gcctabopt > > + > > +@item -mspace > > +@opindex mspace > > +Enable code-size optimizations. > > +Some of these optimizations incur a minor performance penalty. > > We already have -Os, so why is an architecture-specific option for this > needed? > > > +A 16-bit signed constant (-32768..32767) > > Use @minus{} for a minus sign in Texinfo documentation, and @dots{} > instead of literal ".." or "...". I have updated the target options. Space-saving is now enabled by -Os. There is also a new option -msim to enable building for the simulator (the simulator is pending submission to gdb-binutils). I have fixed the Texinfo formatting. FT32 is a new high performance 32-bit RISC core developed by FTDI for embedded applications. Support for FT32 has already been added to binutils. This patch adds FT32 support to gcc. Please can someone review it, and if appropriate commit it, as I do not have write access to the tree. The FSF have acknowledged receipt of FTDI's copyright assignment papers. Thanks very much. ChangeLog entry: 2014-02-16 James Bowman * configure.ac: FT32 target added * libgcc/config.host: FT32 target added * gcc/config/ft32/: FT32 target added * libgcc/config/ft32/: FT32 target added * gcc/doc/install.texi, invoke.texi, md.texi: FT32 details added * gcc/doc/contrib.texi, MAINTAINERS: self added * contrib/config-list.mk: FT32 target added * configure: Regenerated -- James Bowman FTDI Open Source Liaison gcc-ft32.txt.gz Description: gcc-ft32.txt.gz
RE: [PATCH, FT32] initial support
On Wed, 11 Feb 2015, James Bowman wrote: > > > +@table @gcctabopt > > > + > > > +@item -mspace > > > +@opindex mspace > > > +Enable code-size optimizations. > > > +Some of these optimizations incur a minor performance penalty. > > > > We already have -Os, so why is an architecture-specific option for this > > needed? > > Code compiled with -mspace is somewhat slower than code without. > So we typically build *all* code with -Os, with everything > non-critical also compiled -mspace. The typical way of doing that would be to compile the critical code with -O2, everything else with -Os. It's expected -Os may produce slower code than -O2. -- Joseph S. Myers jos...@codesourcery.com
RE: [PATCH, FT32] initial support
> > +@table @gcctabopt > > + > > +@item -mspace > > +@opindex mspace > > +Enable code-size optimizations. > > +Some of these optimizations incur a minor performance penalty. > > We already have -Os, so why is an architecture-specific option for this > needed? Code compiled with -mspace is somewhat slower than code without. So we typically build *all* code with -Os, with everything non-critical also compiled -mspace. Is this a legitimate option or should I just use -Os? -- James Bowman FTDI Open Source Liaison
RE: [PATCH, FT32] initial support
On Wed, 11 Feb 2015, James Bowman wrote: > +@table @gcctabopt > + > +@item -mspace > +@opindex mspace > +Enable code-size optimizations. > +Some of these optimizations incur a minor performance penalty. We already have -Os, so why is an architecture-specific option for this needed? > +A 16-bit signed constant (-32768..32767) Use @minus{} for a minus sign in Texinfo documentation, and @dots{} instead of literal ".." or "...". -- Joseph S. Myers jos...@codesourcery.com
RE: [PATCH, FT32] initial support
(Thanks to everyone for the review. I have reworked the submission as suggested. This does build cleanly with "--enable-werror-always") FT32 is a new high performance 32-bit RISC core developed by FTDI for embedded applications. Support for FT32 has already been added to binutils. This patch adds FT32 support to gcc. Please can someone review it, and if appropriate commit it, as I do not have write access to the tree. The FSF have acknowledged receipt of FTDI's copyright assignment papers. Thanks very much. ChangeLog entry: 2014-02-11 James Bowman * configure.ac: FT32 target added * libgcc/config.host: FT32 target added * gcc/config/ft32/: FT32 target added * libgcc/config/ft32/: FT32 target added * gcc/doc/install.texi, invoke.texi, md.texi: FT32 details added * gcc/doc/contrib.texi, MAINTAINERS: self added * contrib/config-list.mk: FT32 target added * configure: Regenerated -- James Bowman FTDI Open Source Liaison gcc-ft32.txt.gz Description: gcc-ft32.txt.gz
RE: [PATCH, FT32] initial support
> optabs.c's expand_abs_nojump already knows this trick: > > /* If this machine has expensive jumps, we can do integer absolute >value of X as (((signed) x >> (W-1)) ^ x) - ((signed) x >> (W-1)), >where W is the width of MODE. */ > > So if you define BRANCH_COST to be 2 or more there should be no need for > this pattern at all. Yes, I just confirmed this. With no abssi2 pattern, this: int side; int foo(int x) { side = x >> 31; return abs(x); } does indeed produce: foo: ashr.l $r1,$r0,31 sta.l side,$r1 xor.l $r0,$r1,$r0 sub.l $r0,$r0,$r1 return Thanks. -- James Bowman FTDI Open Source Liaison
Re: [PATCH, FT32] initial support
This patch is missing pieces such as Texinfo documentation (in invoke.texi for target-specific options, at least) and config-list.mk update so automatic builders verify that this target builds OK. See "Back End" in sourcebuild.texi and make sure that you have everything relevant. It's a good idea to make sure any new port builds cleanly on both 32-bit and 64-bit hosts when configured --enable-werror-always and the compiler used to build it is the same version of GCC (this provides equivalent coverage for being free of compilation warnings as bootstrap does for native tools). > Index: gcc/config/ft32/ft32.md > === > --- gcc/config/ft32/ft32.md (revision 0) > +++ gcc/config/ft32/ft32.md (revision 0) > @@ -0,0 +1,965 @@ > +;; Machine description for FT32 > +;; Copyright (C) 2009 Free Software Foundation, Inc. Copyright years should be -2015. > +internal_error ("internal error: 'h' applied to non-register > operand"); internal_error already prints "internal compiler error: "; don't include "internal error: " in the error string. Use %< and %> as quotes rather than ''. > +internal_error ("internal error: bad alignment: %d", i); Likewise. > +int bf = ft32_as_bitfield(INTVAL(x)); Missing spaces before '(' in function and macro calls; check for any other instances. > + /* if (REGNO (operand) > FT32_R13) internal_error ("internal error: > bad register: %d", REGNO (operand)); wtf */ Even commented-out calls (if really needed) should be cleaned up. > + if (65536 <= cfun->machine->size_for_adjusting_sp) { > +error("Stack frame must be smaller than 64K"); Start diagnostics with a lowercase letter. Note a missing space again. > +#if 0 > +/* fixed-bit.h does not define functions for TA and UTA because > + that part is wrapped in #if MIN_UNITS_PER_WORD > 4. > + This would lead to empty functions for TA and UTA. > + Thus, supply appropriate defines as if HAVE_[U]TA == 1. > + #define HAVE_[U]TA 1 won't work because avr-modes.def > + uses ADJUST_BYTESIZE(TA,8) and fixed-bit.h is not generic enough > + to arrange for such changes of the mode size. */ Don't include large amounts of #if 0 or commented-out code in a new port (*any* such code needs a good justification). > +ft32-*-elf) > + # tmake_file="ft32/t-ft32 t-softfp-sfdf t-softfp-excl t-softfp" Why commented-out? soft-fp is to be preferred to fp-bit (except for 8-bit and 16-bit ports where space is very critical). Also, t-softfp-excl shouldn't be needed for new ports; it's only really relevant when soft-fp is being built for hard-float multilibs for some reason (t-hardfp is preferred then). If the issue is that you want to exclude some soft-fp functions from being built because you have architecture-specific versions of them, it should be straightforward to add a new softfp_exclusions variable tht t-softfp respects (in a separate patch, please). > + # tm_file="$tm_file ft32/ft32-lib.h" Unless you're using this file, don't include it in the patch at all. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH, FT32] initial support
On 03/02/2015 07:05, Andrew Pinski wrote: > Likewise of: > +(define_insn "abssi2" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (abs:SI (match_operand:SI 1 "register_operand" "r"))) > + (clobber (match_scratch:SI 2 "=&r"))] > + "" > + "ashr.l\t%2,%1,31\;xor.l\t%0,%1,%2\;sub.l\t%0,%0,%2") > optabs.c's expand_abs_nojump already knows this trick: /* If this machine has expensive jumps, we can do integer absolute value of X as (((signed) x >> (W-1)) ^ x) - ((signed) x >> (W-1)), where W is the width of MODE. */ So if you define BRANCH_COST to be 2 or more there should be no need for this pattern at all. Paolo
Re: [PATCH, FT32] initial support
On Mon, Feb 2, 2015 at 9:18 PM, James Bowman wrote: > FT32 is a new high performance 32-bit RISC core developed by FTDI for > embedded applications. > > Support for FT32 has already been added to binutils. This patch adds FT32 > support to gcc. > > Please can someone review it, and if appropriate commit it, as I do not have > write access to the tree. > > The FSF have acknowledged receipt of FTDI's copyright assignment papers. +(define_insn "umulsidi3" + [(set (match_operand:DI 0 "register_operand" "=r,r") +(mult:DI (zero_extend:DI (match_operand:SI 1 "register_operand" "r,r")) + (zero_extend:DI (match_operand:SI 2 "ft32_rimm_operand" "r,KA" + ] + "" + "mul.l $cc,%1,%2\;muluh.l %h0,%1,%2\;move.l %0,$cc") Could you have a split of this instruction to allow better register allocation to happen? Also you are clobbering $cc but don't have a clobber for that register in the pattern. Likewise of: +(define_insn "abssi2" + [(set (match_operand:SI 0 "register_operand" "=r") + (abs:SI (match_operand:SI 1 "register_operand" "r"))) + (clobber (match_scratch:SI 2 "=&r"))] + "" + "ashr.l\t%2,%1,31\;xor.l\t%0,%1,%2\;sub.l\t%0,%0,%2") You also have a few formatting issues dealing with if statements. An example: + /* If this is a store, force the value into a register. */ + if ( 1 && (! (reload_in_progress || reload_completed))) + { +if (MEM_P (operands[0])) +{ it should be: if (!(reload_in_progress || reload_completed)) { if (MEM_P (operands[0])) { + " +{ + /* If this is a store, force the value into a register. */ + if (MEM_P (operands[0])) +operands[1] = force_reg (SFmode, operands[1]); + if (CONST_DOUBLE_P(operands[1])) +operands[1] = force_const_mem(SFmode, operands[1]); +}") You don't need the quotes around the {} in end of the patterns any more. An example: + " +{ + /* If this is a store, force the value into a register. */ + if (MEM_P (operands[0])) +operands[1] = force_reg (SFmode, operands[1]); + if (CONST_DOUBLE_P(operands[1])) +operands[1] = force_const_mem(SFmode, operands[1]); +}") You do some of the instructions have lengths but not all, the ones which matter the most are the ones where the output is more than one instruction. You implement some of the sync_* patterns instead of the newer atomic_* patterns. Thanks, Andrew Pinski > > Thanks very much. ChangeLog entry: > > 2014-02-03 James Bowman > > * configure.ac: FT32 target added > * libgcc/config.host: FT32 target added > * gcc/config/ft32/: FT32 target added > * libgcc/config/ft32/: FT32 target added > * configure: Regenerated > > -- > James Bowman > FTDI Open Source Liaison
[PATCH, FT32] initial support
FT32 is a new high performance 32-bit RISC core developed by FTDI for embedded applications. Support for FT32 has already been added to binutils. This patch adds FT32 support to gcc. Please can someone review it, and if appropriate commit it, as I do not have write access to the tree. The FSF have acknowledged receipt of FTDI's copyright assignment papers. Thanks very much. ChangeLog entry: 2014-02-03 James Bowman * configure.ac: FT32 target added * libgcc/config.host: FT32 target added * gcc/config/ft32/: FT32 target added * libgcc/config/ft32/: FT32 target added * configure: Regenerated -- James Bowman FTDI Open Source Liaison gcc-ft32.txt.gz Description: gcc-ft32.txt.gz