Re: New port^2: Renesas RL78
On 11/28/2011 12:28 PM, DJ Delorie wrote: Ping? Anything else for this? http://gcc.gnu.org/ml/gcc-patches/2011-11/msg02178.html http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01467.html http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01356.html Not that I know of. As far as I'm concerned you can commit the port and handle any follow-ups requested as follow-up patches. r~
Re: New port^2: Renesas RL78
Excellent. Thanks!
Re: New port^2: Renesas RL78
Ping? Anything else for this? http://gcc.gnu.org/ml/gcc-patches/2011-11/msg02178.html http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01467.html http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01356.html
Re: New port^2: Renesas RL78
rl rs, mind sorting this in?
Oops. I'd been putting RL78 before RX for so long it seemed natural
(it's been powerpc so far, which doesn't come between rl78 and rx)
Index: gcc/doc/extend.texi
===
-the SPU and M32C targets support other address spaces. On the SPU target,
for
+the SPU, M32C, and RL78 targets support other address spaces. On the SPU
target, for
Mind the long line.
Reformatted (which is what I was trying to avoid, since now the patch
touches every line in that paragraph)
+On the RL78 target, variables qualified with @code{__far} are accessed
+with 32-bit pointers (20 bit addresses) rather than the default 16-bit
20-bit
Fixed.
+addresses. Non-far variables are assumed to appear in the topmost 64K
+of the address space.
I suggest to explicitly refer to kB or kb (byte or bit) perhaps?
64 kB then.
Same note on sorting as above. :-)
Same fix.
+@node RL78 Options
+@subsection RL78 Options
+@cindex RL78 Options
Ditto.
Ditto.
+@item -msim
+@opindex msim
+Link in additional target libraries to support operation within a
+simulator.
Link, versus...
+@item -mmul=none
+@itemx -mmul=g13
+@itemx -mmul=rl78
+@opindex mmul
+Selects the type of hardware multiplication support desired.
...Selects feels a bit inconsistent, though I can also see the
argument where link is what the program does, whereas select
is what the user does.
I changed Link to Links, and rewrote the Selects line. It looked
like the descriptions are what the options do, not what the user or
gcc does.
Really 1..7, not 0..7? That's unexpected and a bit inconsistent
with Int8.
Yes, really 1..7. It's for shift counts, and they're hard-coded in
the insn (i.e. there are 7 shl insns, 7 shr insns...). The bit
pattern that would be a shift of zero translates into a completely
different insn.
(Reading through this, some very fond memories of the Z80 come up
in my memories. :-)
My first owned computer was a Z80-based Sinclair ZX81 :-)
Don't you want to mention your name there? I would find that
appropriate.
It's not what we typically have done with Red Hat ports.
Do you consider the links from install.texi important? Keep them if
you do, in general we try to minimize those (and keep them in
readings.html where you have added the same links).
No, I was just following the RX entry.
All .texi and .html changes are okay, modulo the notes above.
Updated patches for just docs and www attached.
Index: MAINTAINERS
===
--- MAINTAINERS (revision 181596)
+++ MAINTAINERS (working copy)
@@ -84,12 +84,13 @@ mmix port Hans-Peter Nilsson hp@bitrang
mn10300 port Jeff Lawl...@redhat.com
mn10300 port Alexandre Oliva aol...@redhat.com
moxie port Anthony Green gr...@moxielogic.com
pdp11 port Paul Koning n...@arrl.net
picochip port Hariharan Sandanagobalane hariha...@picochip.com
picochip port Daniel Towner d...@picochip.com
+rl78 port DJ Delorie d...@redhat.com
rs6000 portGeoff Keating geo...@geoffk.org
rs6000 portDavid Edelsohn dje@gmail.com
rs6000 vector extnsAldy Hernandez al...@redhat.com
rx portNick Cliftonni...@redhat.com
s390 port Hartmut Penner hpen...@de.ibm.com
s390 port Ulrich Weigand uweig...@de.ibm.com
Index: gcc/doc/extend.texi
===
--- gcc/doc/extend.texi (revision 181596)
+++ gcc/doc/extend.texi (working copy)
@@ -1217,17 +1217,18 @@ Fixed-point types are supported by the D
@node Named Address Spaces
@section Named address spaces
@cindex named address spaces
As an extension, the GNU C compiler supports named address spaces as
defined in the N1275 draft of ISO/IEC DTR 18037. Support for named
-address spaces in GCC will evolve as the draft technical report changes.
-Calling conventions for any target might also change. At present, only
-the SPU and M32C targets support other address spaces. On the SPU target, for
-example, variables may be declared as belonging to another address space
-by qualifying the type with the @code{__ea} address space identifier:
+address spaces in GCC will evolve as the draft technical report
+changes. Calling conventions for any target might also change. At
+present, only the SPU, M32C, and RL78 targets support other address
+spaces. On the SPU target, for example, variables may be declared as
+belonging to another address space by qualifying the type with the
+@code{__ea} address space identifier:
@smallexample
extern int __ea i;
@end smallexample
When the variable @code{i} is accessed, the
Re: New port^2: Renesas RL78
On Wed, 9 Nov 2011, DJ Delorie wrote:
Index: MAINTAINERS
===
rs6000 port Geoff Keating geo...@geoffk.org
rs6000 port David Edelsohn dje@gmail.com
rs6000 vector extns Aldy Hernandez al...@redhat.com
+rl78 portDJ Delorie d...@redhat.com
rl rs, mind sorting this in?
Index: gcc/doc/extend.texi
===
-the SPU and M32C targets support other address spaces. On the SPU target,
for
+the SPU, M32C, and RL78 targets support other address spaces. On the SPU
target, for
Mind the long line.
+On the RL78 target, variables qualified with @code{__far} are accessed
+with 32-bit pointers (20 bit addresses) rather than the default 16-bit
20-bit
+addresses. Non-far variables are assumed to appear in the topmost 64K
+of the address space.
I suggest to explicitly refer to kB or kb (byte or bit) perhaps?
Index: gcc/doc/invoke.texi
===
* PowerPC Options::
* RS/6000 and PowerPC Options::
+* RL78 Options::
* RX Options::
* S/390 and zSeries Options::
Same note on sorting as above. :-)
+@node RL78 Options
+@subsection RL78 Options
+@cindex RL78 Options
Ditto.
+@item -msim
+@opindex msim
+Link in additional target libraries to support operation within a
+simulator.
Link, versus...
+@item -mmul=none
+@itemx -mmul=g13
+@itemx -mmul=rl78
+@opindex mmul
+Selects the type of hardware multiplication support desired.
...Selects feels a bit inconsistent, though I can also see the
argument where link is what the program does, whereas select
is what the user does.
Index: gcc/doc/md.texi
===
+@item Int3
+An integer constant in the range 1 @dots{} 7.
+@item Int8
+An integer constant in the range 0 @dots{} 255.
Really 1..7, not 0..7? That's unexpected and a bit inconsistent
with Int8. Not sure I have a good alternative to offer, perhaps
someone else has a more creative idea?
(Reading through this, some very fond memories of the Z80 come up
in my memories. :-)
+dtspanRenesas RL78 processor support/span
+span class=date[2011-11-09]/span/dt
+ddA port for the Renesas RL78 family of processors has been contributed by
+Red Hat./dd
Don't you want to mention your name there? I would find that
appropriate.
Do you consider the links from install.texi important? Keep them if
you do, in general we try to minimize those (and keep them in
readings.html where you have added the same links).
All .texi and .html changes are okay, modulo the notes above.
Gerald
Re: New port^2: Renesas RL78
+# non-PIC targets always get an array-bounds error in thread_prologue_and_epilogue_insns +function.o-warn = -Wno-error Didn't we find another way to fix this? In any case this is not present in your changelog. Otherwise the port is looking ok. r~
Re: New port^2: Renesas RL78
Didn't we find another way to fix this? In any case this is not present in your changelog. Yes, please ignore that. I do svn diff and then have to cut out all the bits that aren't part of the base port itself.
Re: New port^2: Renesas RL78
(define_expand umulqihi3 [(set (match_operand:HI 0 register_operand) (mult:HI (zero_extend:HI (match_operand:QI 1 register_operand)) (zero_extend:HI (match_operand:QI 2 register_operand] 0 ) Just delete it? No, we actually have that insn. It's the 0 that needs to be deleted. It sure looks like 99% of the *_real and *_virt insn patterns should have * names so that they don't generate unused expanders. Turns out it was 100%. #define FAILED #define MAYBE_OK(insn) if (insn_ok_now (insn)) return; That non-fallback FAILED probably still ought to be gcc_unreachable. I added that.
Re: New port^2: Renesas RL78
On 11/04/2011 10:09 PM, DJ Delorie wrote: The problem I'm trying to solve with that is that there's only one segment register (ES) so you only need to force an operand non-far if *both* operands are far. Not sure if the function is implemented that way, but I coded the expanders that way. Ah, I missed that. No, the generic code only looks at a single predicate at a time. If you're looking at two, then you do have to take care of it yourself. if (CONST_INT_P (operand1) ! IN_RANGE (INTVAL (operand1), (-1 8) + 1, (1 8) - 1)) FAIL; Huh? This would be an assert, not a FAIL. But why have it? It sounds like something that should have been caught way up the chain... should he says ;-) Indeed. I'll go so far as to say that it's a generic bug that needs fixing. I don't suppose this triggers during a normal build? If so, please file a bug and cc me. (define_expand addsi3 [(set (match_operand:SI 0 register_operand =v) (plus:SI (match_operand:SI 1 nonmemory_operand vi) (match_operand2 nonmemory_operand vi))) ] if (!nonmemory_operand (operands[1], SImode)) operands[1] = force_reg (SImode, operands[1]); if (!nonmemory_operand (operands[1], SImode)) operands[2] = force_reg (SImode, operands[2]); ) Drop the register constrains in the expander. Use register_operand in the expander and drop the force_reg bits. The pattern *does* accept immediates as-is. Not sure why that extra check is in there, though... might be that parts of gcc call gen_addsi3() without checking the predicates first. I've seen it do that for moves. Yes, I suppose that's possible. Especially given add's use inside reload. Yes, it's just like alloca() - it detects a stack shrink in the next call to the library function and frees up any stubs that are off the stack. The call in the epilogue is to increase the odds of properly detecting such a case, since we know at that point it's out of scope. Excellent. r~
Re: New port^2: Renesas RL78
(define_expand zero_extendqihi2 [(set (match_operand:HI 0 nonimmediate_operand) (zero_extend:HI (match_operand:QI 1 general_operand)))] if (rl78_force_nonfar_2 (operands, gen_zero_extendqihi2)) DONE; ) You should be able to simply rl78_nonfar_operand etc and skip the indirect expansion business. I don't know when you started the port but Richard Sandiford's expansion cleanup dated 2011-03-23 should have fixed whatever you're working around in this file. if (CONST_INT_P (operand1) ! IN_RANGE (INTVAL (operand1), (-1 8) + 1, (1 8) - 1)) FAIL; Huh? This would be an assert, not a FAIL. But why have it? It sounds like something that should have been caught way up the chain... (define_expand umulqihi3 [(set (match_operand:HI 0 register_operand) (mult:HI (zero_extend:HI (match_operand:QI 1 register_operand)) (zero_extend:HI (match_operand:QI 2 register_operand] 0 ) Just delete it? (define_expand addsi3 [(set (match_operand:SI 0 register_operand =v) (plus:SI (match_operand:SI 1 nonmemory_operand vi) (match_operand2 nonmemory_operand vi))) ] if (!nonmemory_operand (operands[1], SImode)) operands[1] = force_reg (SImode, operands[1]); if (!nonmemory_operand (operands[1], SImode)) operands[2] = force_reg (SImode, operands[2]); ) Drop the register constrains in the expander. Use register_operand in the expander and drop the force_reg bits. Presumably you're doing the force_reg thing for optimization? Cause it sure looks like your _internal function can handle immediates. It sure looks like you can drop the expander entirely and rename the insn to addsi3. I'm somewhat surprised you don't implement subsi3 as well... It sure looks like 99% of the *_real and *_virt insn patterns should have * names so that they don't generate unused expanders. if (cfun-machine-trampolines_used) emit_insn (gen_trampoline_uninit ()); Do you have another way to uninit, based on stack level? Otherwise this feature is incompatible with longjmp or nonlocal goto. Not that it isn't a fairly decent plan in the circumstances... #define FAILED #define MAYBE_OK(insn) if (insn_ok_now (insn)) return; That non-fallback FAILED probably still ought to be gcc_unreachable. I didn't review the devirt pass in detail. I'm somewhat surprised that you still get decent debug info post-devirt, since I would think you'd need to copy more of the REG_ATTRs data into the new hard regs. The port as a whole looks pretty good though. r~
Re: New port^2: Renesas RL78
(define_expand zero_extendqihi2 [(set (match_operand:HI 0 nonimmediate_operand) (zero_extend:HI (match_operand:QI 1 general_operand)))] if (rl78_force_nonfar_2 (operands, gen_zero_extendqihi2)) DONE; ) You should be able to simply rl78_nonfar_operand etc and skip the indirect expansion business. I don't know when you started the port but Richard Sandiford's expansion cleanup dated 2011-03-23 should have fixed whatever you're working around in this file. The problem I'm trying to solve with that is that there's only one segment register (ES) so you only need to force an operand non-far if *both* operands are far. Not sure if the function is implemented that way, but I coded the expanders that way. if (CONST_INT_P (operand1) ! IN_RANGE (INTVAL (operand1), (-1 8) + 1, (1 8) - 1)) FAIL; Huh? This would be an assert, not a FAIL. But why have it? It sounds like something that should have been caught way up the chain... should he says ;-) (define_expand umulqihi3 [(set (match_operand:HI 0 register_operand) (mult:HI (zero_extend:HI (match_operand:QI 1 register_operand)) (zero_extend:HI (match_operand:QI 2 register_operand] 0 ) Just delete it? Perhaps. Old code, I'd have to compare with the two hardware multipliers (and/or contemplate a libgcc implementation) to see. (define_expand addsi3 [(set (match_operand:SI 0 register_operand =v) (plus:SI (match_operand:SI 1 nonmemory_operand vi) (match_operand2 nonmemory_operand vi))) ] if (!nonmemory_operand (operands[1], SImode)) operands[1] = force_reg (SImode, operands[1]); if (!nonmemory_operand (operands[1], SImode)) operands[2] = force_reg (SImode, operands[2]); ) Drop the register constrains in the expander. Use register_operand in the expander and drop the force_reg bits. The pattern *does* accept immediates as-is. Not sure why that extra check is in there, though... might be that parts of gcc call gen_addsi3() without checking the predicates first. I've seen it do that for moves. I'm somewhat surprised you don't implement subsi3 as well... There's probably a lot of SImode ops I haven't implemented yet. addsi3 was a huge chunk of time in coremark, so it got attention sooner. It sure looks like 99% of the *_real and *_virt insn patterns should have * names so that they don't generate unused expanders. Yup. if (cfun-machine-trampolines_used) emit_insn (gen_trampoline_uninit ()); Do you have another way to uninit, based on stack level? Otherwise this feature is incompatible with longjmp or nonlocal goto. Not that it isn't a fairly decent plan in the circumstances... Yes, it's just like alloca() - it detects a stack shrink in the next call to the library function and frees up any stubs that are off the stack. The call in the epilogue is to increase the odds of properly detecting such a case, since we know at that point it's out of scope. #define FAILED #define MAYBE_OK(insn) if (insn_ok_now (insn)) return; That non-fallback FAILED probably still ought to be gcc_unreachable. No, that's a status indicator for debugging that it hasn't matched *yet*. Probably should get renamed to be more obvious what it's for though ;-) I didn't review the devirt pass in detail. I'm somewhat surprised that you still get decent debug info post-devirt, since I would think you'd need to copy more of the REG_ATTRs data into the new hard regs. I suspect I do, but I don't know how to do that yet. The port as a whole looks pretty good though. Thanks!
