This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: RFA: Add support for Renesas RX architecture to GCC (take 2)
- From: Nick Clifton <nickc at redhat dot com>
- To: Richard Henderson <rth at redhat dot com>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Thu, 15 Oct 2009 09:13:23 +0100
- Subject: Re: RFA: Add support for Renesas RX architecture to GCC (take 2)
- References: <m3r5tefinf.fsf@redhat.com> <4ACFB188.2070709@redhat.com>
Hi Richard,
I think I mentioned this before -- IN_RANGE_P.
Yes you did. I forgot. Sorry. One questions though: IN_RANGE_P is not
defined as a generic macro. It seems that the backends that use it all
define their own, very similar versions. Wouldn't it be simpler to use
the IN_RANGE macro defined in system.h instead ? This is what I have
done in the new patch.
Consider using e.g.
(match_code "plus" "0")
Fascinating. I was unaware of this feature of match_code. Is it
documented somewhere ?
+ static unsigned int
+ bit_count (unsigned int x)
+ {
+ const unsigned int m1 = 0x55555555;
+ const unsigned int m2 = 0x33333333;
+ const unsigned int m4 = 0x0f0f0f0f;
+
+ x -= (x >> 1) & m1;
+ x = (x & m2) + ((x >> 2) & m2);
+ x = (x + (x >> 4)) & m4;
+ x += x >> 8;
+
+ return (x + (x >> 16)) & 0x3f;
We should probably have this function moved to somewhere generic. We
have several places within gcc that we compute a population count.
Search for popcount within the toplevel gcc sources.
Could I submit that as a separate patch ? I would like to keep this one
RX specific.
+ ADD_RX_BUILTIN2 (TST, "tst", void, intSI, intSI);
One last cc0-setting builtin that isn't usable.
Yup. Actually the btst builtin was also guilty of this sin.
+ ADD_RX_BUILTIN1 (PUSHC, "pushc", void, integer);
+ ADD_RX_BUILTIN1 (POPC, "popc", void, integer);
These are a bit questionable, seeing as how they manipulate the stack
register. Consider either removing them, or having them force the
function to use a frame pointer?
I think that removing them would be the safest thing. Even with a frame
pointer we cannot guarantee that the stack pointer will not be used for
addressing some things, so changing it underneath gcc's feet would be a
bad idea.
Aren't there lots more addressing modes that are mode dependant? E.g.
(mem:QI (plus:SI (reg) (const_int 1)))
(mem:SI (plus:SI (reg) (const_int 131074)))
(mem:SI (plus:SI (mult:SI (reg) (const_int 4)))
I.e. the only addresses that aren't mode dependent are plain registers
and reg+const where const is a multiple of 4 and also small enough to
fit a byte offset.
Please move the contents of the macro into a function.
Done.
+ /* Control registers: */ \
+ , { "psw", 0x0 } \
+ , { "pc", 0x1 } \
+ , { "usp", 0x2 } \
+ , { "fpsw", 0x3 } \
+ , { "bpsw", 0x8 } \
+ , { "bpc", 0x9 } \
+ , { "isp", 0xa } \
+ , { "fintv", 0xb } \
+ , { "intb", 0xc } \
These should not be in the same namespace with ADDITONAL_REGISTER_NAMES.
Done.
All of your uses of match_parallel are incorrect. You wind up
generating nested PARALLEL patterns. The only thing that should be
present within a define_insn that uses it is a top-level match_parallel.
This results in patterns like
(define_insn "stack_pushm"
[(match_parallel 1 "rx_pushm_parallel"
[(set (reg:SI SP_REG)
(minus:SI (reg:SI SP_REG)
(match_operand:SI 0 "const_int_operand" "n")))])]
where the stack adjust is pushed inside of the parallel
Done.
That's 'n' not 'i' since you can't negate symbol addresses.
Done.
That said,
I'm not sure you should ever see a (minus reg const_int), since I'm
pretty sure we canonicalize on (plus reg const_int).
I thought so too, but I figured that it would not hurt to provide the
alternative in case there ever was a non-canonical case.
+ (define_insn "addsf3"
+ [(set (match_operand:SF 0 "register_operand" "=r,r,r")
+ (plus:SF (match_operand:SF 1 "register_operand" "%0,0,0")
+ (match_operand:SF 2 "rx_source_operand" "r,F,Q")))]
+ "! TARGET_64BIT_DOUBLES || fast_math_flags_set_p ()"
You took me literally with "-ffast-math" I see. I really expected you
to de-couple -m64bit-double from -mieee. I think -m64bit-double should
not affect SFmode *at all*.
Either you have a -mieee that disables the hw instructions that can't
handle subnormals, or you conditionalize all of these patterns on
flag_unsafe_math_optimizations only. Although really one could argue
for a new flag -f{no-,}subnormal-math that's also adjusted by
-ffast-math instead.
Ah, OK, I think that I understand now. I was unclear as to whether
defining DOUBLE_TYPE_SIZE to be the same as FLOAT_TYPE_SIZE would
automatically make DFmode the same as SFmode.
+ ;; Byte swap (single 32-bit value).
+ (define_insn "revl"
+ [(set (match_operand:SI 0 "register_operand" "+r")
+ (bswap:SI (match_operand:SI 1 "register_operand" "r")))]
+ ""
+ "revl\t%1, %0"
+ [(set_attr "length" "3")]
+ )
s/+/=/
Done.
+ (define_insn "rolc"
+ (define_insn "rorc"
+ (define_insn "satr"
Depends on cc0, and so isn't reliable.
Done.
I am moderately surprised that you aren't representing the accumulator
as a special hard register, kinda like the mips multiplier.
I probably should be. This is only intended to be a base port and I was
not sure exactly how to describe an accumulator to gcc, so I left it
out. Is this a deal-breaker ? I would prefer to leave it out for now
and revisit this at a later date.
+ (define_insn "round"
+ [(set (match_operand:SI 0 "register_operand"
"=r,r")
+ (unspec_volatile:SI [(match_operand:SF 1 "rx_compare_operand"
"r,Q")]
This should be lrintsf2. And you really don't need the volatile.
Done.
+ (define_insn "sync_lock_test_and_setsi"
+ [(parallel
Nested parallel; define_expand needs an explicit parallel, but
define_insn doesn't.
Done.
+ (define_insn "xchg"
Why? You've already got the sync pattern.
Historical - now removed.
Revised patch to follow.
Cheers
Nick