This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFA: Add support for Renesas RX architecture to GCC (take 2)


+ (define_constraint "Int08"
+   "@internal An signed or unsigned 8-bit immediate value"
+   (and (match_code "const_int")
+        (and (match_test "ival >= -256")
+           (match_test "ival <   256")
+        )
+   )
+ )

I think I mentioned this before -- IN_RANGE_P.


+ (define_memory_constraint "Q"
+   "A MEM which does not use REG+REG, REG+(REG*SCALE), --REG or REG++ addressing."
+   (and (match_code "mem")
+        (ior (and (match_test "GET_CODE (XEXP (op,0)) == PLUS")
+                (and (match_test "RX_REG_P (XEXP (XEXP (op, 0), 0))")
+                     (match_test "CONST_INT_P (XEXP (XEXP (op, 0), 1))")
+                )
+           )
+          (match_test "GET_CODE (XEXP (op, 0)) == REG")
+        )
+    )
+ )

Consider using e.g.


 (match_code "plus" "0")
 (match_code "reg" "00")
 (match_code "const_int" "01")
 (match_code "reg" "0")

+ 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.


+ ADD_RX_BUILTIN2 (TST, "tst", void, intSI, intSI);

One last cc0-setting builtin that isn't usable.


+   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?


+ /* These addressing modes only work for SImode.  */
+ #define GO_IF_MODE_DEPENDENT_ADDRESS(ADDR, LABEL)                     \
+   do                                                                  \
+     {                                                                 \
+       if (GET_CODE (ADDR) == PRE_DEC || GET_CODE (ADDR) == POST_INC)  \
+       goto LABEL;                                                     \
+       if (GET_CODE (ADDR) == PLUS                                     \
+         && REG_P (XEXP (ADDR, 0))                                     \
+         && REG_P (XEXP (ADDR, 1)))                                    \
+       goto LABEL;                                                     \
+     }                                                                 \
+   while (0)

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.

+     /* 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.


+ (define_insn "stack_pushm"
+   [(set:SI (reg:SI SP_REG)
+          (minus:SI (reg:SI SP_REG)
+                    (match_operand:SI        0 "immediate_operand" "i")))
+    (match_parallel                            1 "rx_store_multiple_vector"
+                  [(set:SI (match_operand:SI 2 "memory_operand"   "=m")
+                           (match_operand:SI 3 "register_operand"  "r"))])]

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

+ (define_insn "subsi3"
+   [(set (match_operand:SI           0 "register_operand" "=r,r,r,r,r")
+       (minus:SI (match_operand:SI 1 "register_operand"  "0,0,0,r,0")
+                 (match_operand:SI 2 "rx_source_operand" "r,Uint04,i,r,Q")))]
+   ""
+   "@
+   sub\t%2, %0
+   sub\t%2, %0
+   add\t%N2, %0

That's 'n' not 'i' since you can't negate symbol addresses. 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).


+ (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.

+ ;; 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/+/=/


+ (define_insn "rolc"
+ (define_insn "rorc"
+ (define_insn "satr"

Depends on cc0, and so isn't reliable.


I am moderately surprised that you aren't representing the accumulator as a special hard register, kinda like the mips multiplier.

+ (define_insn "round"
+   [(set (match_operand:SI                      0 "register_operand"  "=r,r")
+       (unspec_volatile:SI [(match_operand:SF 1 "rx_compare_operand" "r,Q")]
+                           UNSPEC_BUILTIN_ROUND))]
+   ""
+   "round\t%1, %0"
+   [(set_attr "cc" "set_zs")
+    (set_attr "timings" "22,44")
+    (set_attr "length" "3,5")]
+ )

This should be lrintsf2. And you really don't need the volatile.


+ (define_insn "sync_lock_test_and_setsi"
+   [(parallel

Nested parallel; define_expand needs an explicit parallel, but define_insn doesn't.


+ (define_insn "xchg"

Why? You've already got the sync pattern.




r~


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]