From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6338 invoked by alias); 1 Mar 2016 15:15:41 -0000 Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org Received: (qmail 6307 invoked by uid 89); 1 Mar 2016 15:15:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.9 required=5.0 tests=AWL,BAYES_50,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=NONE, selective, 0x07, fitted X-HELO: mail-wm0-f51.google.com Received: from mail-wm0-f51.google.com (HELO mail-wm0-f51.google.com) (74.125.82.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 01 Mar 2016 15:15:36 +0000 Received: by mail-wm0-f51.google.com with SMTP id n186so41887466wmn.1 for ; Tue, 01 Mar 2016 07:15:35 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=yuMEOm/IavPNUxOxFiNhix9WX37RBgEdF5PFUxWeNM4=; b=XJW8mqtcczgarLNujCwrfNh6L5nilaNiiNyCr0VJYkkkw5jQlqxGrCLY3HxMkSCmkk CLzEknoAyDcg05Wid0NrknvIJIH7rSDesH/ucSfNNpJ6IopGm1h3WAOPkU10BlrTMATQ dcwfl5cbWxLRCOlg+gfSYwar4gn+iWpcd7oT7c6ahjOwbsr42bUsBl9X/+vEtQ9a6cnm 9LCH3HWHHf9o6QNhAhuugjoeKYeN35kl9jPYd/Z+yY5yS6mu6W2rOKuKSRJqFT6+4Txo dEj47mL9E0LvD36W+hOGjL9absV/vgcMombQTYBWHX3ot9KBYfZEWvW4332kdfq8lTlb TMCA== X-Gm-Message-State: AD7BkJKT6UtZqqSzFOOp1X5drSymsrzpDi+RzOtWAblQTFdIB+Ec/IdhG2CQXI958T73UQ== X-Received: by 10.194.8.38 with SMTP id o6mr21226257wja.31.1456845333165; Tue, 01 Mar 2016 07:15:33 -0800 (PST) Received: from localhost (host86-138-94-184.range86-138.btcentralplus.com. [86.138.94.184]) by smtp.gmail.com with ESMTPSA id c7sm21691474wmd.13.2016.03.01.07.15.31 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 01 Mar 2016 07:15:32 -0800 (PST) Date: Tue, 01 Mar 2016 15:15:00 -0000 From: Andrew Burgess To: Claudiu Zissulescu Cc: "binutils@sourceware.org" , "noamca@mellanox.com" , "Cupertino.Miranda@synopsys.com" Subject: Re: [PATCH] gas/opcodes: Add initial arc nps400 support Message-ID: <20160301151522.GQ9275@embecosm.com> References: <1456443554-23475-1-git-send-email-andrew.burgess@embecosm.com> <098ECE41A0A6114BB2A07F1EC238DE8966179CDB@DE02WEMBXB.internal.synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <098ECE41A0A6114BB2A07F1EC238DE8966179CDB@DE02WEMBXB.internal.synopsys.com> X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.5.24 (2015-08-30) X-IsSubscribed: yes X-SW-Source: 2016-03/txt/msg00009.txt.bz2 * Claudiu Zissulescu [2016-02-29 11:00:05 +0000]: Hi Claudiu, Thanks for your feedback. I have some follow on questions: > > > > +/* ARC NPS400 support. */ > > +#undef ARC_NPS400 > > + > My opinion is we do not need this define, or at least not in this > form. If we want to support multiple selective cpu targets, maybe we > should (un)define something like ARC_CPU_DEFAULT. Where this macro > will be initialized with a desired cpu by the configure script > file. This may make sense for example when the assembler needs to > behave differently between two cpu configurations, but as far as I > can see there is not the case for the time being. So do you see the 'ARC_CPU_DEFAULT' being defined to 'ARC700' for the current stock arc binutils, and then set to 'NPS400' for the Mellanox NPS400 binutils? The name feels wrong to me, the default for both core arc, and nps400 would still be arc700, it's just in the nps400 case we have an additional feature enabled; one that adds some additional instructions. If the default architecture for arc changes to arc700 as you suggest below, then all uses of ARC_NPS400 disappear from gas, so the config changes can also be dropped. That leaves just a single use of ARC_NPS400 in the opcode library, where we include the extension instructions. One possibility would be that I could create a define call ARC_OPCODE_EXTENSION_INSN_FILE or some such. This would be undefined for core arc, and defined to "arc-nps400-tbl.h" for nps400, then in arc_opcodes[] we can say: #ifdef ARC_OPCODE_EXTENSION_INSN_FILE #include ARC_OPCODE_EXTENSION_INSN_FILE #endif Would that be better? If you still prefer ARC_CPU_DEFAULT, could you expand a little more on how you see it being used please. > > > + if (!mach_type_specified_p) > > + { > > +#ifdef ARC_NPS400 > > + arc_select_cpu ("arc700"); > > +#else > > + arc_select_cpu ("all"); > > +#endif > > + } > You can let the default cpu to arc700. The "all" stuff is just for > testing purposes, and needed to be removed anyhow. OK, I'll do that. > > > + arc) > > + case ${target} in > > + arc*-mellanox-*) > > + > > +$as_echo "#define ARC_NPS400 1" >>confdefs.h > > + > > + ;; > > + esac > > + ;; > > + > See my above comments. > > > + arc) > > + case ${target} in > > + arc*-mellanox-*) > > + AC_DEFINE(ARC_NPS400, 1, [ARC NPS400 support.]) > > + ;; > > + esac > > + ;; > > + > Likewise. > > > +++ b/opcodes/arc-nps400-tbl.h > > @@ -0,0 +1,3 @@ > > +/* movb<.f><.cl> */ > > +{ "movb", 0x48010000, 0xf80f0000, ARC_OPCODE_ARC700 , ARITH, NONE, { > > +NPS_R_DST, NPS_R_SRC1, NPS_R_SRC2, NPS_BITOP_DST_POS, > > +NPS_BITOP_SRC_POS, NPS_BITOP_SIZE }, { C_NPS_F }}, { "movb", > > +0x48010000, 0xf80f0000, ARC_OPCODE_ARC700 , ARITH, NONE, { > > NPS_R_J_DST, > > +NPS_R_SRC2, NPS_BITOP_DST_POS, NPS_BITOP_SRC_POS, > > NPS_BITOP_SIZE }, { > > +C_NPS_F, C_NPS_CL }}, > > Probably the best will be to make a file common for all other users > of ARC architecture which are having their own custom > instructions. Hence, I would propose to rename arc-nps400-tbl.h to > arc-ext-tbl.h or something of a sort. I'm not sure I like that idea. Keeping different extensions separate would be best I think. The total number of extensions is likely to be pretty low, but the number of entries in (this one at least) is likely to be ~100. Keeping them separate will help provide more structure to the code, making it more obvious which sets of extensions go together. > Also, if you would like those > instructions to be enabled by a particular assembler option, you can > use a new Subclass category (e.g., add NPS400 to insn_subclass_t) > instead of NONE, and match against it. It may make sense as your > opcodes are overlapping with other arc instructions. Which instructions do they overlap with? I thought that these new instructions fitted into the gap in the arc architecture reserved for after-market extensions (major opcode 0x09). As an experiment I assembled the nps400 new instruction test case using the nps400 binutils then disassemble using a clean, stock arc binutils, all I see are ".long HEX-VALUE" lines, which is what I would expect. The desired behaviour at this stage is that a binutils configured for nps400 would appear to be an arc700 binutils, just with the additional instruction included. > In this > situation, the disassembler may choose the first ones instead of > yours. However, in this case, we need to add an option to the > disassembler as well. If this ever became a problem then I think the correct solution would be a new ELF header flag, in the same way we split ARCv2 in to ARCEM and ARCHS, we'd split ARC700 into ARC700 and NPS400. I'm hoping any such flag changes can be put off until necessary. > > > +/* ARC NPS400 Support: See comment near head of file. */ static > > +unsigned insert_nps_dst (unsigned insn ATTRIBUTE_UNUSED, > > + int value ATTRIBUTE_UNUSED, > > + const char **errmsg ATTRIBUTE_UNUSED) { > > + switch (value) > > + { > > + case 0: > > + case 1: > > + case 2: > > + case 3: > > + insn |= value << 24; > > + break; > > + case 12: > > + case 13: > > + case 14: > > + case 15: > > + insn |= (value - 8) << 24; > > + break; > > + default: > > + *errmsg = _("Register must be either r0-r3 or r12-r15."); > > + break; > > + } > > + return insn; > > +} > > + > > +static int > > +extract_nps_dst (unsigned insn ATTRIBUTE_UNUSED, > > + bfd_boolean * invalid ATTRIBUTE_UNUSED) { > > + int value = (insn >> 24) & 0x07; > > + if (value > 3) > > + return (value + 8); > > + else > > + return value; > > +} > > + > > +static unsigned > > +insert_nps_j_dst (unsigned insn ATTRIBUTE_UNUSED, > > + int value ATTRIBUTE_UNUSED, > > + const char **errmsg ATTRIBUTE_UNUSED) { > > + switch (value) > > + { > > + case 0: > > + case 1: > > + case 2: > > + case 3: > > + insn |= value << 24; > > + break; > > + case 12: > > + case 13: > > + case 14: > > + case 15: > > + insn |= (value - 8) << 24; > > + break; > > + default: > > + *errmsg = _("Register must be either r0-r3 or r12-r15."); > > + break; > > + } > > + return insn; > > +} > This function looks similar with insert_nps_dst(). Is not possible > to use a single function only? I'll try to get get more code reuse into here where possible. It sounds like you're happy with the general approach, which is the main thing :) I'll post a revised patch set soon. Thanks, Andrew