From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11208 invoked by alias); 9 Oct 2009 21:56:38 -0000 Received: (qmail 11198 invoked by uid 22791); 9 Oct 2009 21:56:37 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 09 Oct 2009 21:56:32 +0000 Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n99LuV7Z011899 for ; Fri, 9 Oct 2009 17:56:31 -0400 Received: from stone.twiddle.home (vpn-226-41.phx2.redhat.com [10.3.226.41]) by int-mx04.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id n99LuUa1003306; Fri, 9 Oct 2009 17:56:30 -0400 Message-ID: <4ACFB188.2070709@redhat.com> Date: Fri, 09 Oct 2009 21:57:00 -0000 From: Richard Henderson User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.1) Gecko/20090814 Fedora/3.0-2.6.b3.fc11 Thunderbird/3.0b3 MIME-Version: 1.0 To: Nick Clifton CC: gcc-patches@gcc.gnu.org Subject: Re: RFA: Add support for Renesas RX architecture to GCC (take 2) References: In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2009-10/txt/msg00634.txt.bz2 > + (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~