From: Doug Evans <dje@transmeta.com>
To: Dave Brolley <brolley@redhat.com>
Cc: cgen@sources.redhat.com
Subject: [patch][rfa] Ordering insns in hash chain for cgen disassemblers
Date: Tue, 09 Oct 2001 16:29:00 -0000 [thread overview]
Message-ID: <15346.55680.817467.321067@casey.transmeta.com> (raw)
In-Reply-To: <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;
WARNING: multiple messages have this Message-ID
From: Doug Evans <dje@transmeta.com>
To: Dave Brolley <brolley@redhat.com>
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 [thread overview]
Message-ID: <15346.55680.817467.321067@casey.transmeta.com> (raw)
Message-ID: <20011114125200.HqjwW3E87HhG_L3fRMPQ46tKSSxX3Do3ue-3MMC0KjI@z> (raw)
In-Reply-To: <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;
next prev parent reply other threads:[~2001-11-14 20:52 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2001-10-05 13:20 Dave Brolley
2001-10-09 12:09 ` [patch][rfa] Ordering insns in hash chain for cgen disassemblers -- committed Dave Brolley
2001-11-14 12:09 ` Dave Brolley
2001-10-09 16:29 ` Doug Evans [this message]
2001-10-10 2:11 ` [patch][rfa] Ordering insns in hash chain for cgen disassemblers Dave Brolley
2001-10-10 3:33 ` Doug Evans
2001-10-10 6:33 ` Dave Brolley
2001-11-15 10:11 ` Dave Brolley
2001-11-15 9:52 ` Doug Evans
[not found] ` <15348.168.750723.846784.cygnus.local.cgen@casey.transmeta.com>
2001-10-13 2:39 ` Frank Ch. Eigler
2001-11-15 12:42 ` Frank Ch. Eigler
2001-11-15 9:21 ` Dave Brolley
2001-11-14 12:52 ` Doug Evans
2001-11-13 11:46 ` Dave Brolley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=15346.55680.817467.321067@casey.transmeta.com \
--to=dje@transmeta.com \
--cc=brolley@redhat.com \
--cc=cgen@sources.redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).