From: Yao Qi <qiyaoltc@gmail.com>
To: Omair Javaid <omair.javaid@linaro.org>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v4 4/5] Support for recording aarch64 advanced SIMD instructions
Date: Fri, 20 Feb 2015 09:53:00 -0000 [thread overview]
Message-ID: <867fvczz7a.fsf@gmail.com> (raw)
In-Reply-To: <1422926216-15740-5-git-send-email-omair.javaid@linaro.org> (Omair Javaid's message of "Tue, 3 Feb 2015 06:16:55 +0500")
Omair Javaid <omair.javaid@linaro.org> writes:
Hi Omair,
you need some descriptions about your patch rather than just a changelog
here.
> gdb:
>
> 2015-02-02 Omair Javaid <omair.javaid@linaro.org>
>
> * aarch64-tdep.c (aarch64_record_data_proc_simd_fp): Add handler
> for data processing SIMD and floating point insns.
"New function" should be fine.
> (aarch64_record_asimd_load_store): Add handler to record ASIMD load
> store insns.
Likewise.
> (aarch64_record_load_store): Install record handler
> aarch64_record_asimd_load_store.
> (aarch64_record_decode_insn_handler): Install record handler
> aarch64_record_data_proc_simd_fp.
> ---
> gdb/aarch64-tdep.c | 228 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 226 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index faa650a..d2ec2c1 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3047,6 +3047,144 @@ aarch64_record_branch_except_sys (insn_decode_record *aarch64_insn_r)
> return AARCH64_RECORD_SUCCESS;
> }
>
> +/* Record handler for advanced SIMD load and store instructions. */
A blank line is needed here.
> +static unsigned int
> +aarch64_record_asimd_load_store (insn_decode_record *aarch64_insn_r)
> +{
> + CORE_ADDR address;
> + uint64_t addr_offset = 0;
> + uint32_t record_buf[24];
> + uint64_t record_buf_mem[24];
> + uint32_t reg_rn, reg_rt, reg_rm;
> + uint32_t reg_index = 0, mem_index = 0;
> + uint8_t eindex, rindex, sindex, reg_tt, replicate;
> + uint8_t elements, esize, rpt, selem, single, scale;
> + uint8_t opcode_bits, size_bits, ld_flag, data_size, wback;
There are so many local variables defined. Please define them in the
block in which they are used.
> +
> + reg_rt = bits (aarch64_insn_r->aarch64_insn, 0, 4);
> + reg_rn = bits (aarch64_insn_r->aarch64_insn, 5, 9);
> + reg_rm = bits (aarch64_insn_r->aarch64_insn, 16, 20);
> +
> + wback = bit (aarch64_insn_r->aarch64_insn, 23);
> + single = bit (aarch64_insn_r->aarch64_insn, 24);
> + ld_flag = bit (aarch64_insn_r->aarch64_insn, 22);
> + size_bits = bits (aarch64_insn_r->aarch64_insn, 10, 11);
> + opcode_bits = bits (aarch64_insn_r->aarch64_insn, 12, 15);
> + regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rn, &address);
> +
> + if (single)
Nit: since 'single' is only used once, better to use
'bit (aarch64_insn_r->aarch64_insn, 24)', however this is just my
personal preference.
> + {
Add a comment here like /* Load/store single structure */
> + scale = opcode_bits >> 2;
scale and replicate can be defined here.
> + selem = ((opcode_bits & 0x02) |
> + bit (aarch64_insn_r->aarch64_insn, 21)) + 1;
> + replicate = 0;
> + switch (scale)
> + {
> + case 2:
> + if (!(size_bits & 0x01) && ((size_bits >> 1) & 0x01))
> + scale = 3;
> + break;
> + case 3:
> + scale = size_bits;
> + replicate = 1;
> + break;
> + default:
> + break;
> + }
Sorry, I don't understand this piece of code. Could you elaborate?
This block is supposed to handle "Advanced SIMD load/store single
structure" and "Advanced SIMD load/store single structure
(post-indexed)", right?
> + esize = 8 << scale;
> + if (replicate)
> + for (sindex = 0; sindex < selem; sindex++)
> + {
> + record_buf[reg_index++] = reg_rt + AARCH64_V0_REGNUM;
> + reg_rt = (reg_rt + 1) % 32;
> + }
> + else
> + {
> + for (sindex = 0; sindex < selem; sindex++)
> + if (ld_flag)
> + record_buf[reg_index++] = reg_rt + AARCH64_V0_REGNUM;
> + else
> + {
> + record_buf_mem[mem_index++] = esize / 8;
> + record_buf_mem[mem_index++] = address + addr_offset;
> + }
> + addr_offset = addr_offset + (esize / 8);
> + reg_rt = (reg_rt + 1) % 32;
> + }
> + }
> + else
> + {
local variables elements and rpt can be defined here, with comments.
> + esize = 8 << size_bits;
> + if (bit (aarch64_insn_r->aarch64_insn, 30))
> + elements = 128 / esize;
> + else
> + elements = 64 / esize;
> +
> + switch (opcode_bits)
> + {
> + case 0:
Is it "ST4 (multiple structures)" instruction? If it is, add comments
for each case about what instructions it is to handle.
> + rpt = 1;
> + selem = 4;
> + break;
> + case 2:
> + rpt = 4;
> + selem = 1;
> + break;
> + case 4:
> + rpt = 1;
> + selem = 3;
> + break;
> + case 6:
> + rpt = 3;
> + selem = 1;
> + break;
> + case 7:
> + rpt = 1;
> + selem = 1;
> + break;
> + case 8:
> + rpt = 1;
> + selem = 2;
> + break;
> + case 10:
> + rpt = 2;
> + selem = 1;
> + break;
> + default:
> + return AARCH64_RECORD_UNSUPPORTED;
> + break;
> + }
> + for (rindex = 0; rindex < rpt; rindex++)
> + for (eindex = 0; eindex < elements; eindex++)
> + {
> + reg_tt = (reg_rt + rindex) % 32;
> + for (sindex = 0; sindex < selem; sindex++)
> + {
> + if (ld_flag)
The whole "else" branch is about "Advanced SIMD load/store multiple
structures" and "Advanced SIMD load/store multiple structures
(post-indexed)" instructions, IIUC. How can ld_flag (bit 22) be true?
It is always zero as far as I can tell from the manual.
> +
> +/* Record handler for data processing SIMD and floating point instructions. */
> +
> +static unsigned int
> +aarch64_record_data_proc_simd_fp (insn_decode_record *aarch64_insn_r)
> +{
> + uint8_t insn_bit21, opcode, rmode, reg_rd;
> + uint8_t insn_bits24_27, insn_bits28_31, insn_bits10_11, insn_bits12_15;
> + uint8_t insn_bits11_14;
> + uint32_t record_buf[2];
> +
> + insn_bits24_27 = bits (aarch64_insn_r->aarch64_insn, 24, 27);
> + insn_bits28_31 = bits (aarch64_insn_r->aarch64_insn, 28, 31);
> + insn_bits10_11 = bits (aarch64_insn_r->aarch64_insn, 10, 11);
> + insn_bits12_15 = bits (aarch64_insn_r->aarch64_insn, 12, 15);
> + insn_bits11_14 = bits (aarch64_insn_r->aarch64_insn, 11, 14);
> + opcode = bits (aarch64_insn_r->aarch64_insn, 16, 18);
> + rmode = bits (aarch64_insn_r->aarch64_insn, 19, 20);
> + reg_rd = bits (aarch64_insn_r->aarch64_insn, 0, 4);
> + insn_bit21 = bit (aarch64_insn_r->aarch64_insn, 21);
> +
> + if ((insn_bits28_31 & 0x05) == 0x01 && insn_bits24_27 == 0x0e)
> + {
> + /* Floating point - fixed point conversion instructions. */
> + if (!insn_bit21)
brace is needed here...
> + if ((opcode >> 1) == 0x0 && rmode == 0x03)
> + record_buf[0] = reg_rd;
> + else
> + record_buf[0] = reg_rd + AARCH64_V0_REGNUM;
... and here.
> + /* Floating point - conditional compare instructions. */
> + else if (insn_bits10_11 == 0x01)
> + record_buf[0] = AARCH64_CPSR_REGNUM;
> + /* Floating point - data processing (2-source) and
> + conditional select instructions. */
> + else if (insn_bits10_11 == 0x02 || insn_bits10_11 == 0x03)
> + record_buf[0] = reg_rd + AARCH64_V0_REGNUM;
> + else if (insn_bits10_11 == 0x00)
> + {
> + /* Floating point - immediate instructions. */
> + if ((insn_bits12_15 & 0x01) == 0x01 || (insn_bits12_15 & 0x07) == 0x04)
> + record_buf[0] = reg_rd + AARCH64_V0_REGNUM;
> + /* Floating point - compare instructions. */
> + else if ((insn_bits12_15 & 0x03) == 0x02)
> + record_buf[0] = AARCH64_CPSR_REGNUM;
> + /* Floating point - integer conversions instructions. */
> + if (insn_bits12_15 == 0x00)
> + {
> + /* Convert float to integer instruction. */
> + if (!(opcode >> 1) || ((opcode >> 1) == 0x02 && !rmode))
> + record_buf[0] = reg_rd + AARCH64_X0_REGNUM;
> + /* Convert integer to float instruction. */
> + else if ((opcode >> 1) == 0x01 && !rmode)
> + record_buf[0] = reg_rd + AARCH64_V0_REGNUM;
> + /* Move float to integer instruction. */
> + else if ((opcode >> 1) == 0x03)
> + {
> + if (!(opcode & 0x01))
> + record_buf[0] = reg_rd + AARCH64_X0_REGNUM;
> + else
> + record_buf[0] = reg_rd + AARCH64_V0_REGNUM;
> + }
> + }
> + }
> + }
> + else if ((insn_bits28_31 & 0x09) == 0x00 && insn_bits24_27 == 0x0E)
s/0x0E/0x0e/
> + {
> + /* Advanced SIMD copy instructions. */
> + if (!bits (aarch64_insn_r->aarch64_insn, 21, 23) &&
Move && to the next line.
> + !bit (aarch64_insn_r->aarch64_insn, 15) &&
> + bit (aarch64_insn_r->aarch64_insn, 10))
brace here
> + if (insn_bits11_14 == 0x05 || insn_bits11_14 == 0x07)
> + record_buf[0] = reg_rd + AARCH64_X0_REGNUM;
> + else
> + record_buf[0] = reg_rd + AARCH64_V0_REGNUM;
and here.
> + else
> + record_buf[0] = reg_rd + AARCH64_V0_REGNUM;
> + }
> + /* All remaining floating point or advanced SIMD instructions. */
> + else
> + record_buf[0] = reg_rd + AARCH64_V0_REGNUM;
> +
> + REG_ALLOC (aarch64_insn_r->aarch64_regs, aarch64_insn_r->reg_rec_count,
> + record_buf);
> + return AARCH64_RECORD_SUCCESS;
> +}
> +
Last but not least, we need test case for these Advanced SIMD
instructions, as compiler may not generate these instructions for
gdb.reverse/*.c files. x86 has already had some tests, so aarch64
should have some too. As a start, we don't need to cover all Advanced
SIMD instructions in one go.
--
Yao (齐尧)
next prev parent reply other threads:[~2015-02-20 9:53 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-03 1:17 [PATCH v4 0/5] Process record and reverse debugging support on aarch64-linux Omair Javaid
2015-02-03 1:17 ` [PATCH v4 1/5] NEWS entry about aarch64-linux record/replay support Omair Javaid
2015-02-03 3:38 ` Eli Zaretskii
2015-02-03 1:17 ` [PATCH v4 4/5] Support for recording aarch64 advanced SIMD instructions Omair Javaid
2015-02-20 9:53 ` Yao Qi [this message]
2015-02-03 1:17 ` [PATCH v4 2/5] Implements aarch64 process record and reverse debugging support Omair Javaid
2015-02-19 12:35 ` Yao Qi
2015-02-03 1:17 ` [PATCH v4 5/5] Enables gdb.reverse testsuite for aarch64*-linux targets Omair Javaid
2015-02-20 9:57 ` Yao Qi
2015-02-03 1:17 ` [PATCH v4 3/5] Support for recording syscall on aarch64-linux Omair Javaid
2015-02-19 16:55 ` Yao Qi
2015-02-03 1:32 ` [PATCH v4 0/5] Process record and reverse debugging support " Omair Javaid
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=867fvczz7a.fsf@gmail.com \
--to=qiyaoltc@gmail.com \
--cc=gdb-patches@sourceware.org \
--cc=omair.javaid@linaro.org \
/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).