From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1750 invoked by alias); 14 Nov 2001 20:52:49 -0000 Mailing-List: contact cgen-help@sourceware.cygnus.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: cgen-owner@sourceware.cygnus.com Received: (qmail 1728 invoked from network); 14 Nov 2001 20:52:48 -0000 From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <15346.55680.817467.321067@casey.transmeta.com> Date: Tue, 09 Oct 2001 16:29:00 -0000 To: Dave Brolley Cc: cgen@sources.redhat.com Subject: [patch][rfa] Ordering insns in hash chain for cgen disassemblers In-Reply-To: <3BF178A6.F12C7023@redhat.com> References: <3BF178A6.F12C7023@redhat.com> X-Mailer: VM 6.72 under 21.1 (patch 8) "Bryce Canyon" XEmacs Lucid X-SW-Source: 2001-q4/txt/msg00005.txt.bz2 Dave Brolley writes: > Currently insns are added to the chains of the hash table in the > disassembler in the order in which they appear in the .cpu file. Thus, > resolution of insns which are a special case of another must be > managed by carefully defining then in the correct order. The insns are > also parsed in the order in which they appear and this is important in > cases where an operand in the assembler input might be successfully > parsed as more than on ecgen operand (e.g. register names are > successfully parsed as immediates). There is an impass, however, when > the order in which insns need to be parsed is not the same as the > order in which decoding must be attempted. > > I have run into such a case where an operand of an insn may be a > register name or an immediate. One particular register is forbidden, > however, and when the bits representing that register appear in the > field of the insn, it signifies that the operand is really an > immediate value which follows the insn. Thus the operand must be > parsed as a register name first, but decoding must attempt it as an > immediate first. > > The problem may be solved by automating the order in which the insns > are placed into the hash chains in the disassembler. By ordering insns > iby the number of decoding bits in decreasing order, we can assure > that an insn which is a special case of another wil be attempted > first, regardless of the order in which they appear in the .cpu file. > This is the same ordering which would have been required manually up > until now, so no existing ports should be affected. Setting aside the state of the implementation of ifield assertions (since I don't remember what it is), why wouldn't an ifield assertion work here? I'm not saying I disapprove of the patch in principle, just curious whether you tried that and the results of it. Studying your patch though, I have some questions. Are you assuming both insns are on the same hash chain? [If not, never mind of course.] If so, is that guaranteed? Is basing the sort strictly on the number of decodable bit sufficient? Also, style nit: I hate the absence of a blank line between the comment introducing the function and the function definition. Another style note while I'm on it. Boy do I hate the proliferation of "pretty aligned code". int a; some_long_type_definition_t b; (or see the static fn decls at the top of cgen-dis.c) puhleeze. :-( > Index: opcodes/cgen-dis.c > =================================================================== > RCS file: /cvs/src/src/opcodes/cgen-dis.c,v > retrieving revision 1.5 > diff -c -p -r1.5 cgen-dis.c > *** opcodes/cgen-dis.c 2001/09/19 17:40:28 1.5 > --- opcodes/cgen-dis.c 2001/11/12 20:10:55 > *************** > *** 30,36 **** > static CGEN_INSN_LIST * hash_insn_array PARAMS ((CGEN_CPU_DESC, const CGEN_INSN *, int, int, CGEN_INSN_LIST **, CGEN_INSN_LIST *)); > static CGEN_INSN_LIST * hash_insn_list PARAMS ((CGEN_CPU_DESC, const CGEN_INSN_LIST *, CGEN_INSN_LIST **, CGEN_INSN_LIST *)); > static void build_dis_hash_table PARAMS ((CGEN_CPU_DESC)); > ! > /* Subroutine of build_dis_hash_table to add INSNS to the hash table. > > COUNT is the number of elements in INSNS. > --- 30,87 ---- > static CGEN_INSN_LIST * hash_insn_array PARAMS ((CGEN_CPU_DESC, const CGEN_INSN *, int, int, CGEN_INSN_LIST **, CGEN_INSN_LIST *)); > static CGEN_INSN_LIST * hash_insn_list PARAMS ((CGEN_CPU_DESC, const CGEN_INSN_LIST *, CGEN_INSN_LIST **, CGEN_INSN_LIST *)); > static void build_dis_hash_table PARAMS ((CGEN_CPU_DESC)); > ! > ! /* Return the number of decodable bits in this insn. */ > ! static int > ! count_decodable_bits (insn) > ! const CGEN_INSN *insn; > ! { > ! unsigned mask = CGEN_INSN_BASE_MASK (insn); > ! int bits = 0; > ! int m; > ! for (m = 1; m != 0; m <<= 1) > ! { > ! if (mask & m) > ! ++bits; > ! } > ! return bits; > ! } > ! > ! /* Add an instruction to the hash chain. */ > ! static void > ! add_insn_to_hash_chain (hentbuf, insn, htable, hash) > ! CGEN_INSN_LIST *hentbuf; > ! const CGEN_INSN *insn; > ! CGEN_INSN_LIST **htable; > ! unsigned int hash; > ! { > ! CGEN_INSN_LIST *current_buf; > ! CGEN_INSN_LIST *previous_buf; > ! int insn_decodable_bits; > ! > ! /* Add insns sorted by the number of decodable bits, in decreasing order. > ! This ensures that any insn which is a special case of another will be > ! checked first. */ > ! insn_decodable_bits = count_decodable_bits (insn); > ! previous_buf = NULL; > ! for (current_buf = htable[hash]; current_buf != NULL; > ! current_buf = current_buf->next) > ! { > ! int current_decodable_bits = count_decodable_bits (current_buf->insn); > ! if (insn_decodable_bits >= current_decodable_bits) > ! break; > ! previous_buf = current_buf; > ! } > ! > ! /* Now insert the new insn. */ > ! hentbuf->insn = insn; > ! hentbuf->next = current_buf; > ! if (previous_buf == NULL) > ! htable[hash] = hentbuf; > ! else > ! previous_buf->next = hentbuf; > ! } > ! > /* Subroutine of build_dis_hash_table to add INSNS to the hash table. > > COUNT is the number of elements in INSNS. > *************** hash_insn_array (cd, insns, count, entsi > *** 74,82 **** > CGEN_INSN_MASK_BITSIZE (insn), > big_p); > hash = (* cd->dis_hash) (buf, value); > ! hentbuf->next = htable[hash]; > ! hentbuf->insn = insn; > ! htable[hash] = hentbuf; > } > > return hentbuf; > --- 125,131 ---- > CGEN_INSN_MASK_BITSIZE (insn), > big_p); > hash = (* cd->dis_hash) (buf, value); > ! add_insn_to_hash_chain (hentbuf, insn, htable, hash); > } > > return hentbuf; > *************** hash_insn_list (cd, insns, htable, hentb > *** 114,122 **** > CGEN_INSN_MASK_BITSIZE (ilist->insn), > big_p); > hash = (* cd->dis_hash) (buf, value); > ! hentbuf->next = htable [hash]; > ! hentbuf->insn = ilist->insn; > ! htable [hash] = hentbuf; > } > > return hentbuf; > --- 163,169 ---- > CGEN_INSN_MASK_BITSIZE (ilist->insn), > big_p); > hash = (* cd->dis_hash) (buf, value); > ! add_insn_to_hash_chain (hentbuf, ilist->insn, htable, hash); > } > > return hentbuf; From mboxrd@z Thu Jan 1 00:00:00 1970 From: Doug Evans To: Dave Brolley Cc: cgen@sources.redhat.com Subject: [patch][rfa] Ordering insns in hash chain for cgen disassemblers Date: Wed, 14 Nov 2001 12:52:00 -0000 Message-ID: <15346.55680.817467.321067@casey.transmeta.com> References: <3BF178A6.F12C7023@redhat.com> X-SW-Source: 2001-q4/msg00030.html Message-ID: <20011114125200.HqjwW3E87HhG_L3fRMPQ46tKSSxX3Do3ue-3MMC0KjI@z> Dave Brolley writes: > Currently insns are added to the chains of the hash table in the > disassembler in the order in which they appear in the .cpu file. Thus, > resolution of insns which are a special case of another must be > managed by carefully defining then in the correct order. The insns are > also parsed in the order in which they appear and this is important in > cases where an operand in the assembler input might be successfully > parsed as more than on ecgen operand (e.g. register names are > successfully parsed as immediates). There is an impass, however, when > the order in which insns need to be parsed is not the same as the > order in which decoding must be attempted. > > I have run into such a case where an operand of an insn may be a > register name or an immediate. One particular register is forbidden, > however, and when the bits representing that register appear in the > field of the insn, it signifies that the operand is really an > immediate value which follows the insn. Thus the operand must be > parsed as a register name first, but decoding must attempt it as an > immediate first. > > The problem may be solved by automating the order in which the insns > are placed into the hash chains in the disassembler. By ordering insns > iby the number of decoding bits in decreasing order, we can assure > that an insn which is a special case of another wil be attempted > first, regardless of the order in which they appear in the .cpu file. > This is the same ordering which would have been required manually up > until now, so no existing ports should be affected. Setting aside the state of the implementation of ifield assertions (since I don't remember what it is), why wouldn't an ifield assertion work here? I'm not saying I disapprove of the patch in principle, just curious whether you tried that and the results of it. Studying your patch though, I have some questions. Are you assuming both insns are on the same hash chain? [If not, never mind of course.] If so, is that guaranteed? Is basing the sort strictly on the number of decodable bit sufficient? Also, style nit: I hate the absence of a blank line between the comment introducing the function and the function definition. Another style note while I'm on it. Boy do I hate the proliferation of "pretty aligned code". int a; some_long_type_definition_t b; (or see the static fn decls at the top of cgen-dis.c) puhleeze. :-( > Index: opcodes/cgen-dis.c > =================================================================== > RCS file: /cvs/src/src/opcodes/cgen-dis.c,v > retrieving revision 1.5 > diff -c -p -r1.5 cgen-dis.c > *** opcodes/cgen-dis.c 2001/09/19 17:40:28 1.5 > --- opcodes/cgen-dis.c 2001/11/12 20:10:55 > *************** > *** 30,36 **** > static CGEN_INSN_LIST * hash_insn_array PARAMS ((CGEN_CPU_DESC, const CGEN_INSN *, int, int, CGEN_INSN_LIST **, CGEN_INSN_LIST *)); > static CGEN_INSN_LIST * hash_insn_list PARAMS ((CGEN_CPU_DESC, const CGEN_INSN_LIST *, CGEN_INSN_LIST **, CGEN_INSN_LIST *)); > static void build_dis_hash_table PARAMS ((CGEN_CPU_DESC)); > ! > /* Subroutine of build_dis_hash_table to add INSNS to the hash table. > > COUNT is the number of elements in INSNS. > --- 30,87 ---- > static CGEN_INSN_LIST * hash_insn_array PARAMS ((CGEN_CPU_DESC, const CGEN_INSN *, int, int, CGEN_INSN_LIST **, CGEN_INSN_LIST *)); > static CGEN_INSN_LIST * hash_insn_list PARAMS ((CGEN_CPU_DESC, const CGEN_INSN_LIST *, CGEN_INSN_LIST **, CGEN_INSN_LIST *)); > static void build_dis_hash_table PARAMS ((CGEN_CPU_DESC)); > ! > ! /* Return the number of decodable bits in this insn. */ > ! static int > ! count_decodable_bits (insn) > ! const CGEN_INSN *insn; > ! { > ! unsigned mask = CGEN_INSN_BASE_MASK (insn); > ! int bits = 0; > ! int m; > ! for (m = 1; m != 0; m <<= 1) > ! { > ! if (mask & m) > ! ++bits; > ! } > ! return bits; > ! } > ! > ! /* Add an instruction to the hash chain. */ > ! static void > ! add_insn_to_hash_chain (hentbuf, insn, htable, hash) > ! CGEN_INSN_LIST *hentbuf; > ! const CGEN_INSN *insn; > ! CGEN_INSN_LIST **htable; > ! unsigned int hash; > ! { > ! CGEN_INSN_LIST *current_buf; > ! CGEN_INSN_LIST *previous_buf; > ! int insn_decodable_bits; > ! > ! /* Add insns sorted by the number of decodable bits, in decreasing order. > ! This ensures that any insn which is a special case of another will be > ! checked first. */ > ! insn_decodable_bits = count_decodable_bits (insn); > ! previous_buf = NULL; > ! for (current_buf = htable[hash]; current_buf != NULL; > ! current_buf = current_buf->next) > ! { > ! int current_decodable_bits = count_decodable_bits (current_buf->insn); > ! if (insn_decodable_bits >= current_decodable_bits) > ! break; > ! previous_buf = current_buf; > ! } > ! > ! /* Now insert the new insn. */ > ! hentbuf->insn = insn; > ! hentbuf->next = current_buf; > ! if (previous_buf == NULL) > ! htable[hash] = hentbuf; > ! else > ! previous_buf->next = hentbuf; > ! } > ! > /* Subroutine of build_dis_hash_table to add INSNS to the hash table. > > COUNT is the number of elements in INSNS. > *************** hash_insn_array (cd, insns, count, entsi > *** 74,82 **** > CGEN_INSN_MASK_BITSIZE (insn), > big_p); > hash = (* cd->dis_hash) (buf, value); > ! hentbuf->next = htable[hash]; > ! hentbuf->insn = insn; > ! htable[hash] = hentbuf; > } > > return hentbuf; > --- 125,131 ---- > CGEN_INSN_MASK_BITSIZE (insn), > big_p); > hash = (* cd->dis_hash) (buf, value); > ! add_insn_to_hash_chain (hentbuf, insn, htable, hash); > } > > return hentbuf; > *************** hash_insn_list (cd, insns, htable, hentb > *** 114,122 **** > CGEN_INSN_MASK_BITSIZE (ilist->insn), > big_p); > hash = (* cd->dis_hash) (buf, value); > ! hentbuf->next = htable [hash]; > ! hentbuf->insn = ilist->insn; > ! htable [hash] = hentbuf; > } > > return hentbuf; > --- 163,169 ---- > CGEN_INSN_MASK_BITSIZE (ilist->insn), > big_p); > hash = (* cd->dis_hash) (buf, value); > ! add_insn_to_hash_chain (hentbuf, ilist->insn, htable, hash); > } > > return hentbuf;