This is the mail archive of the
cgen@sources.redhat.com
mailing list for the CGEN project.
[patch][rfa] Ordering insns in hash chain for cgen disassemblers
- From: Doug Evans <dje at transmeta dot com>
- To: Dave Brolley <brolley at redhat dot com>
- Cc: cgen at sources dot redhat dot com
- Date: Wed, 14 Nov 2001 12:52:16 -0800 (PST)
- Subject: [patch][rfa] Ordering insns in hash chain for cgen disassemblers
- References: <3BF178A6.F12C7023@redhat.com>
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;