From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6062 invoked by alias); 14 Jan 2011 18:48:23 -0000 Received: (qmail 6050 invoked by uid 22791); 14 Jan 2011 18:48:21 -0000 X-SWARE-Spam-Status: No, hits=-6.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD 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, 14 Jan 2011 18:48:15 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id p0EIm6XV031611 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 14 Jan 2011 13:48:06 -0500 Received: from anchor.twiddle.home (ovpn-113-146.phx2.redhat.com [10.3.113.146]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p0EIm5ov017093; Fri, 14 Jan 2011 13:48:05 -0500 Message-ID: <4D309A65.4080008@redhat.com> Date: Fri, 14 Jan 2011 18:54:00 -0000 From: Richard Henderson User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20101209 Fedora/3.1.7-0.35.b3pre.fc14 Thunderbird/3.1.7 MIME-Version: 1.0 To: Sumanth Gundapaneni CC: Joseph Myers , "gcc-patches@gcc.gnu.org" , "Jayant R. Sonar" Subject: Re: CR16 Port addition References: <371569CBCFB2E745B891DBB88B2DFDDD19EB4868CF@KCINPUNHJCMS01.kpit.com> In-Reply-To: <371569CBCFB2E745B891DBB88B2DFDDD19EB4868CF@KCINPUNHJCMS01.kpit.com> Content-Type: text/plain; charset=ISO-8859-1 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: 2011-01/txt/msg01011.txt.bz2 > +/* Initialize 'targetm' variable which contains pointers to functions > + and data relating to the target machine. */ > +struct gcc_target targetm = TARGET_INITIALIZER; Not a requirement, but as a comment: I find it much cleaner to move the target initializer to the end of the file, which avoids the need for forward delcarations of the functions that are placed within. I know there is a lot of variation in this among ports, but this is something that ought to get cleaned up in the future. > +cr16_class_likely_spilled_p (reg_class_t rclass) > +{ > + if ((rclass) == SHORT_REGS || (rclass) == DOUBLE_BASE_REGS \ Trailing \. Left over from macro-to-function conversion? > + /* To avoid gen_rtx_SUBREG ICE. SI gets splitted to two HIs where > + validate_subreg is invalid with the second HI having offset 2 > + .Offset applicable to big endian machines. */ > + flag_split_wide_types = 0; I would really like to know more about this. > + /* If EH, disable optimization GCSE. */ > + if (flag_exceptions) > + flag_gcse = 0; Why? It works for other targets. > +/* Compute values for the array save_regs and the variable sum_regs. > + The index of save_regs is numbers of register, each will get 1 if we > + need to save it in the current function, 0 if not. sum_regs is the > + total sum of the registers being saved. */ > +static void > +cr16_compute_save_regs (void) ... > +/* Compute the size of the local area and the size to be adjusted by the > + prologue and epilogue. */ > +static void > +cr16_compute_frame (void) I would be very much happier if these functions did not store their results into global variables. Consider using a structure, like struct ix86_frame is used in the i386 port. > + if ((mode == Pmode) && (regno == 11)) > + return 0; Is there a symbolic name for 11? > +cr16_function_arg (CUMULATIVE_ARGS * cum, enum machine_mode mode, > + const_tree type, bool named ATTRIBUTE_UNUSED) > +{ > + last_parm_in_reg = 0; Using a global here is definitely wrong. If you need to store extra data, then store it in the CUMULATIVE_ARGS structure. Though in this case I think all you need to do is share some code between function_arg and function_arg_advance. > + if ((next_param == (tree) 0) && (TREE_VALUE (param) != void_type_node)) s/(tree) 0/NULL_TREE/g > +/* Constraint for bit instructions. */ > +#define OK_FOR_Z(OP) \ > + ((GET_CODE (OP) == MEM && GET_CODE (XEXP (OP, 0)) == CONST_INT) \ > + || (GET_CODE (OP) == MEM && GET_CODE (XEXP (OP, 0)) == REG) \ > + || (GET_CODE (OP) == MEM && GET_CODE (XEXP (OP, 0)) == PLUS \ > + && GET_CODE (XEXP ((XEXP (OP, 0)), 0)) == REG \ > + && GET_CODE (XEXP ((XEXP (OP, 0)), 1)) == CONST_INT)) ... > +(define_predicate "bit_operand" > + (match_code "mem") > +{ > + return ((GET_CODE (op) == MEM && OK_FOR_Z (op))); > +}) These should be merged, since OK_FOR_Z isn't used elsewhere. I don't see a constraints.md file? Or any of the legacy macros for that matter. Did the file get forgotten in the patch? > + [(set (match_operand:SHORT 0 "bit_operand" "=mZ") The constraint here is almost certainly wrong. It should be only "=Z". If you're having problems with this, it means you havn't properly defined the Z constraint with define_memory_constraint. > +;; Register operand to avoid subreg with offset byte offset 2 > +(define_predicate "reg_operand" > + (match_operand 0 "register_operand") > +{ > + if (GET_CODE(op) == SUBREG > + && (REGNO(SUBREG_REG(op)) > 11 > + && REGNO(SUBREG_REG(op)) < FIRST_PSEUDO_REGISTER) > + && SUBREG_BYTE(op) != 0) > + return 0; > + return 1; > +}) This is wrong. This issue should be handled with CANNOT_CHANGE_MODE_CLASS, and not with random hacks across the rest of the port. > +(define_expand "adddi3" > + [(set (match_operand:DI 0 "reg_operand" "") > + (plus:DI (match_operand:DI 1 "reg_operand" "") > + (match_operand:DI 2 "reg_operand" "")))] > + "" > + { > + if ((GET_CODE (operands[2]) != REG) ) > + { > + operands[2] = force_reg (DImode, operands[2]); > + } > + } You should trust the predicates to do the right thing. You have constrained operands[2] to be a reg_operand. Why are you then checking that it's not a REG? Indeed, once that's fixed there's no reason not to simply merge your adddi3 expander with the adddi3_insn pattern. > + (plus:SI (mult:SI (sz_xtnd:SI (match_operand:HI 1 "reg_operand" "%r")) > + (sz_xtnd:SI (match_operand:HI 2 "reg_operand" "r"))) % should be used only when the constraints of the two operands are different. Otherwise you're just wasting time in reload making it examine both possibilities. > + [(set (match_operand:HI 0 "reg_operand" "=r,=r") > + (mult:HI (match_operand:HI 1 "register_and_valid_subreg_byte_operand" "%0,0") > + (match_operand:HI 2 "nonmemory_operand" "r,i")))] Extra = in operand 0. > +(define_expand "andhi3" > + [(set (match_operand:HI 0 "reg_operand" "") > + (and:HI (match_operand:HI 1 "reg_operand" "") > + (match_operand:HI 2 "nonmemory_operand" "")))] > + "" > + " > +{ > + if ((GET_CODE (operands[1]) == SUBREG > + && REGNO (operands[1]) >= FIRST_PSEUDO_REGISTER) > + && (GET_CODE (operands[2]) == SUBREG > + && REGNO (operands[2]) >= FIRST_PSEUDO_REGISTER)) > + FAIL; > +}") > + What is this for? Disallowing subregs of pseudos?!? > +;;T - prints upper half > +;;U - prints lower half > +(define_insn "anddi3" > + [(set (match_operand:DI 0 "reg_operand" "=r") > + (and:DI (match_operand:DI 1 "reg_operand" "%0") > + (match_operand:DI 2 "nonmemory_operand" "r")))] > + "" > + "andd\t%T2,%T0\;andd\t%U2,%U0" > +) You'd be better off without this, and let the compiler generate it itself from two SImode ands. Likewise with the rest of the logical arithmetic. > +; (DI, DF) move > +(define_insn "*mov_double" > + [(set (match_operand:DOUBLE 0 "nonimmediate_operand" "=r, r, r, m") > + (match_operand:DOUBLE 1 "gen_operand" "r, , m, r"))] > + "register_operand (operands[0], DImode) > + || register_operand (operands[0], DFmode) > + || register_operand (operands[1], DImode) > + || register_operand (operands[1], DFmode)" This whole pattern has got to get cleaned up. The best way to do so is to delete it, and let the generic bits of the compiler handle this itself via subreg decomposition. Of course, this requires that you fix the subreg problems that you're having rather than papering over. > +(define_insn "cbranch4" > + [(set (pc) > + (if_then_else (match_operator 0 "ordered_comparison_operator" > + [(match_operand:CR16IM 1 "register_operand" "r,r") > + (match_operand:CR16IM 2 "nonmemory_operand" "r,n")]) > + (label_ref (match_operand 3 "" "")) > + (pc))) > + (clobber (cc0))] > + "" > + "cmp\t%2, %1\;b%d0\t%l3" > + [(set_attr "length" "6,6")] > +) > + > +(define_expand "cmp" > + [(parallel [(set (cc0) > + (compare (match_operand:CR16IM 0 "register_operand" "") > + (match_operand:CR16IM 1 "register_operand" ""))) > + (clobber (match_scratch:HI 2 "=r"))]) ] > + "" > + "") > + > +(define_insn "*cmp_insn" > + [(set (cc0) > + (compare (match_operand:CR16IM 0 "register_operand" "r,r") > + (match_operand:CR16IM 1 "nonmemory_operand" "r,n"))) > + (clobber (match_scratch:HI 2 "=r,r")) ] > + "" > + "cmp\t%1, %0" > + [(set_attr "length" "2,4")] > +) I'm not sure I understand the logic here. If you have a cbranch insn, which you never split, I'm not sure why you'd need a cmp pattern at all. > +(define_insn "indirect_jump_return" > + [(parallel > + [(set (pc) > + (reg:SI RA_REGNUM)) > + (return)]) > + ] ... > +(define_insn "interrupt_return" > + [(parallel > + [(unspec_volatile [(const_int 0)] 0) > + (return)])] ... > +(define_insn "push_for_prologue" > + [(parallel > + [(set (reg:SI SP_REGNUM) > + (minus:SI (reg:SI SP_REGNUM) > + (match_operand:SI 0 "immediate_operand" "i")))])] ... > +(define_insn "pop_and_popret_return" > + [(parallel > + [(set (reg:SI SP_REGNUM) > + (plus:SI (reg:SI SP_REGNUM) > + (match_operand:SI 0 "immediate_operand" "i"))) > + (use (reg:SI RA_REGNUM)) > + (return)]) Extra parallel. It's implicit in define_insn. > +(define_expand "call" > + [(call (match_operand:QI 0 "memory_operand" "") > + (match_operand 1 "" ""))] > + "" ... > +(define_expand "cr16_call" > + [(parallel > + [(call (match_operand:QI 0 "memory_operand" "") > + (match_operand 1 "" "")) > + (clobber (reg:SI RA_REGNUM))])] These can be trivially merged. > +;; Nop > +(define_insn "nop" > + [(const_int 0)] > + "" > + "" > +) Nop really needs to output something. It's used to make sure various line numbers are distinct addresses at -O0 -g. > +;; Shared FLAT > +(define_insn "unspec_library_offset" > + [(const_int UNSPEC_LIBRARY_OFFSET)] > + "" > + "push \t$2, r0;\n\tmovd \t$__current_shared_library_r12_offset_, (r1,r0);\n\tloadd \t[r12]0(r1,r0), (r12);\n\tpop \t$2, r0\n\t" > + [(set_attr "length" "12")] > +) > + > +(define_insn "unspec_sh_lib_push_r12" > + [(const_int UNSPEC_SH_LIB_PUSH_R12)] > + "" > + "push \t$2, r12" > + [(set_attr "length" "2")] > +) > + > +(define_insn "unspec_sh_lib_pop_r12" > + [(const_int UNSPEC_SH_LIB_POP_R12)] > + "" > + "pop \t$2, r12" > + [(set_attr "length" "2")] > +) Huh? r~