i#3700: Add IR for AArch64's SIMD LD1/ST1 instructions #3710

AssadHashmi · 2019-06-28T09:41:37Z

These are the multiple single element structure load/stores
from/to one vector register variants:

ST1 { <Vt>.<T> }, [<Xn|SP>]
LD1 { <Vt>.<T> }, [<Xn|SP>]

These are the multiple single element structure load/stores from/to one vector register variants: ST1 { <Vt>.<T> }, [<Xn|SP>] LD1 { <Vt>.<T> }, [<Xn|SP>]

AssadHashmi · 2019-06-28T10:47:30Z

run arm tests

derekbruening

Just some style nits. I did not look in the manual to check the encoding bits. Are there tests, either automated or manual pre-commit, to ensure this is the right encoding by comparing to some other decoder?

derekbruening · 2019-06-28T15:50:44Z

core/arch/aarch64/instr_create.h

+
+/**
+ * Creates an Advanced SIMD (NEON) LD1 instruction to load multiple
+ * single element structures to one vector register, e.g. LD1 {V0.4H},[X0]


nit: missing trailing .

derekbruening · 2019-06-28T15:50:56Z

core/arch/aarch64/instr_create.h

+
+/**
+ * Creates an Advanced SIMD (NEON) ST1 instruction to store multiple
+ * single element structures from one vector register, e.g. ST1 {V1.2S},[X1]


nit: missing trailing .

derekbruening · 2019-06-28T15:55:48Z

core/arch/aarch64/instr_create.h

+/* TODO: Remaining advanced SIMD (NEON) memory instructions:
+#define INSTR_CREATE_ld2/3/4_multi_2/3/4()
+#define INSTR_CREATE_ld1/2/3/4_single()
+and st1 equivalents including post-index variants */


style: Add the issue number, so TODO i#2626: .

Also we prefer */ on its own line.

fhahn · 2019-06-28T16:31:34Z

Great, thanks Assad! Did you use the scripts to auto-generate them?

fhahn · 2019-06-28T16:47:59Z

suite/tests/api/ir_aarch64.c

+       INSTR_CREATE_st1_multi_<n>() where <n> is 1, 2, 3 or 4
+
+       ST1 { <Vt>.<T> }, [<Xn|SP>]
+       ST1 { <Vt>.<T>, <Vt2>.<T> }, [<Xn|SP>]


Currently, only the 1 operand version is tested, right?

Yes. Hopefully more will follow as time allows.

Hmm...good spot. Perhaps I should remove those comments as they could be misleading?

Maybe not. The header comments are useful placeholders and each test call is explicitly commented to make it clear it's the 1 operand version being run.

fhahn · 2019-06-28T16:48:07Z

suite/tests/api/ir_aarch64.c

+       INSTR_CREATE_ld1_multi_<n>() where <n> is 1, 2, 3 or 4
+
+       LD1 { <Vt>.<T> }, [<Xn|SP>]
+       LD1 { <Vt>.<T>, <Vt2>.<T> }, [<Xn|SP>]


Currently, only the 1 operand version is tested, right?

Yes. Hopefully more will follow as time allows.

Hmm...good spot. Perhaps I should remove those comments as they could be misleading?

Maybe not. The header comments are useful placeholders and each test call is explicitly commented to make it clear it's the 1 operand version being run.

fhahn · 2019-06-28T16:48:20Z

core/arch/aarch64/codec.txt

@@ -756,7 +756,7 @@ x0110111xxxxxxxxxxxxxxxxxxxxxxxx  tbnz   tbz
 0x001100000000000010xxxxxxxxxxxx  st1    memvm : vmsz vt0 vt1 vt2 vt3
 0x001100000000000100xxxxxxxxxxxx  st3    memvm : vmsz vt0 vt1 vt2
 0x001100000000000110xxxxxxxxxxxx  st1    memvm : vmsz vt0 vt1 vt2
-0x001100000000000111xxxxxxxxxxxx  st1    memvm : vmsz vt0
+0x001100000000000111xxxxxxxxxxxx  st1    memvm : vt0 vmsz


Did we flip the position of the vmsz operand here to be more in line with the XXX_sz operands, which come last?

Makes sense! Is there a reason you did not change it for ld1?

Makes sense! Is there a reason you did not change it for ld1?

ld1 doesn't need to change as its fields are already correctly positioned w.r.t. encoding string:

0x001100010000000111xxxxxxxxxxxx ld1 vt0 : memvm vmsz

AssadHashmi · 2019-06-28T16:58:40Z

Just some style nits. I did not look in the manual to check the encoding bits. Are there tests, either automated or manual pre-commit, to ensure this is the right encoding by comparing to some other decoder?

In this case I've tested the instruction macros in a client which copies vectors between buffers. It's an application specific client. At some point I'll port it to a client targeted at testing which is safe to be upstreamed as part of the test suite. It can be the basis of further AArch64 SIMD work.

AssadHashmi · 2019-06-28T17:04:13Z

Great, thanks Assad! Did you use the scripts to auto-generate them?

No, unfortunately. There's still some issues and work required and resourcing is . . . variable ;-)

fhahn · 2019-06-28T17:43:59Z

suite/tests/api/dis-a64.txt

@@ -74,7 +74,7 @@
 0b9f13ff : add    wzr, wzr, wzr, asr #4   : add    %wzr %wzr asr $0x04 -> %wzr
 0c0007ff : st4    {v31.4h, v0.4h, v1.4h, v2.4h}, [sp]: st4    $0x01 %d31 %d0 %d1 %d2 -> (%sp)[32byte]
 0c0067ff : st1    {v31.4h, v0.4h, v1.4h}, [sp]: st1    $0x01 %d31 %d0 %d1 -> (%sp)[24byte]
-0c0077ff : st1    {v31.4h}, [sp]          : st1    $0x01 %d31 -> (%sp)[8byte]
+0c0077ff : st1    {v31.4h}, [sp]          : st1    %d31 $0x01 -> (%sp)[8byte]


Would it be possible to add tests for all sizes? same for ld1.

fhahn · 2019-06-28T17:44:22Z

core/arch/aarch64/instr.c

@@ -335,6 +335,12 @@ reg_is_gpr(reg_id_t reg)
    return (DR_REG_X0 <= reg && reg <= DR_REG_WSP);
 }

+bool
+reg_is_simd(reg_id_t reg)


I added reg_is_simd() in order for AArch64 to conform to the API.

I see. Would it be possible to add a test for it to https://github.com/DynamoRIO/dynamorio/blob/master/suite/tests/api/opnd-a64.c ?

fhahn · 2019-06-28T17:46:06Z

core/arch/aarch64/codec.txt

@@ -756,7 +756,7 @@ x0110111xxxxxxxxxxxxxxxxxxxxxxxx  tbnz   tbz
 0x001100000000000010xxxxxxxxxxxx  st1    memvm : vmsz vt0 vt1 vt2 vt3
 0x001100000000000100xxxxxxxxxxxx  st3    memvm : vmsz vt0 vt1 vt2
 0x001100000000000110xxxxxxxxxxxx  st1    memvm : vmsz vt0 vt1 vt2
-0x001100000000000111xxxxxxxxxxxx  st1    memvm : vmsz vt0
+0x001100000000000111xxxxxxxxxxxx  st1    memvm : vt0 vmsz


Makes sense! Is there a reason you did not change it for ld1?

fhahn · 2019-07-02T13:27:43Z

core/arch/aarch64/codec.txt

@@ -756,7 +756,7 @@ x0110111xxxxxxxxxxxxxxxxxxxxxxxx  tbnz   tbz
 0x001100000000000010xxxxxxxxxxxx  st1    memvm : vmsz vt0 vt1 vt2 vt3
 0x001100000000000100xxxxxxxxxxxx  st3    memvm : vmsz vt0 vt1 vt2
 0x001100000000000110xxxxxxxxxxxx  st1    memvm : vmsz vt0 vt1 vt2
-0x001100000000000111xxxxxxxxxxxx  st1    memvm : vmsz vt0
+0x001100000000000111xxxxxxxxxxxx  st1    memvm : vt0 vmsz


fhahn · 2019-07-02T13:27:49Z

core/arch/aarch64/instr.c

@@ -335,6 +335,12 @@ reg_is_gpr(reg_id_t reg)
    return (DR_REG_X0 <= reg && reg <= DR_REG_WSP);
 }

+bool
+reg_is_simd(reg_id_t reg)


I see. Would it be possible to add a test for it to https://github.com/DynamoRIO/dynamorio/blob/master/suite/tests/api/opnd-a64.c ?

fhahn · 2019-07-02T13:35:51Z

core/arch/aarch64/instr.c

+bool
+reg_is_simd(reg_id_t reg)
+{
+    return (DR_REG_Q0 <= reg && reg <= DR_REG_B31);


I am a bit surprised that S, H and B registers are included here. I thought the only vector registers used by instructions are the 128 bit Q, as in v8.2d, v8.4s, v8.8h and 64 bit D as in v8.2s, v8.4h. Am I missing something and are there encodings that use S, H or B registers as vector register?

My (simple) understanding is that all reg_is_simd() is saying is whether a given register is SIMD or not, and B/H/S/D are components of a SIMD Q register. That's independent of any encoding issues. Have I missed something?

derekbruening · 2019-07-05T15:46:37Z

Remember the 'Fixes #NNN' or 'Issue: #NNN' for future commits: this one had neither and so has no link to the issue.

Can #3700 be closed now?

AssadHashmi · 2019-07-05T16:54:03Z

Remember the 'Fixes #NNN' or 'Issue: #NNN' for future commits: this one had neither and so has no link to the issue.

Gah! My bad.

Can #3700 be closed now?

Not yet. There are still variants of the SIMD ld/st which need to be implemented and the test coverage increased in general.

derekbruening · 2019-07-05T17:23:59Z

Remember the 'Fixes #NNN' or 'Issue: #NNN' for future commits: this one had neither and so has no link to the issue.

Gah! My bad.

If you've run the devsetup script, it sets a commit message template which makes it easier to remember by pre-populating each commit message with the suggested form.

Minor conflict in core/arch/aarch64/codec.txt due to PR: #3710

This is an internal implementation of a feature we would like to enable in upstream DynamoRIO. IMO proposing such a change upstream is more efficient if a working example is available. Once this is merged an IP clean proposal patch can be created which represents the interface we would like. This patch required an upstream change: #3710 Change-Id: Ifbb14e21083dc68d50609401032ddc2d40c9f7fc Reviewed-on: https://eu-gerrit-1.euhpc.arm.com/193795 Reviewed-by: Al Grant <al.grant@arm.com> Tested-by: mdc-bbot

AssadHashmi added 2 commits June 28, 2019 10:36

i#3700: Add IR for AArch64's SIMD LD1/ST1 instructions

15c7775

These are the multiple single element structure load/stores from/to one vector register variants: ST1 { <Vt>.<T> }, [<Xn|SP>] LD1 { <Vt>.<T> }, [<Xn|SP>]

Fixed formatting violations spotted by Travis checks

35ea2d0

AssadHashmi requested review from fhahn and egrimley June 28, 2019 11:15

derekbruening approved these changes Jun 28, 2019

View reviewed changes

Addressed review comments

9421e96

fhahn reviewed Jun 28, 2019

View reviewed changes

Merge branch 'master' into i3700-aarch64-simd-ld1-st1

230cd04

fhahn reviewed Jul 2, 2019

View reviewed changes

Merge branch 'master' into i3700-aarch64-simd-ld1-st1

c06c4b8

AssadHashmi merged commit bb78eb5 into master Jul 3, 2019

AssadHashmi deleted the i3700-aarch64-simd-ld1-st1 branch July 5, 2019 16:56

AssadHashmi mentioned this pull request Jul 29, 2019

Add AArch64 support DynamoRIO/drmemory#2016

Open

egrimley-arm pushed a commit that referenced this pull request Nov 26, 2024

[ARMIE-164] Merged latest upstream head to prep for 19.2

b4275ea

Minor conflict in core/arch/aarch64/codec.txt due to PR: #3710

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i#3700: Add IR for AArch64's SIMD LD1/ST1 instructions #3710

i#3700: Add IR for AArch64's SIMD LD1/ST1 instructions #3710

AssadHashmi commented Jun 28, 2019

AssadHashmi commented Jun 28, 2019

derekbruening left a comment

derekbruening Jun 28, 2019

derekbruening Jun 28, 2019

derekbruening Jun 28, 2019

fhahn commented Jun 28, 2019

fhahn Jun 28, 2019

AssadHashmi Jun 28, 2019

AssadHashmi Jun 28, 2019

AssadHashmi Jun 28, 2019

fhahn Jun 28, 2019

AssadHashmi Jun 28, 2019

AssadHashmi Jun 28, 2019

AssadHashmi Jun 28, 2019

fhahn Jun 28, 2019

AssadHashmi Jun 28, 2019

fhahn Jun 28, 2019

AssadHashmi Jul 1, 2019

fhahn Jul 2, 2019

AssadHashmi commented Jun 28, 2019

AssadHashmi commented Jun 28, 2019

fhahn Jun 28, 2019

fhahn Jun 28, 2019

AssadHashmi Jul 1, 2019

fhahn Jul 2, 2019

fhahn Jun 28, 2019

fhahn Jul 2, 2019

fhahn Jul 2, 2019

fhahn Jul 2, 2019

AssadHashmi Jul 3, 2019

derekbruening commented Jul 5, 2019

AssadHashmi commented Jul 5, 2019 •

edited

Loading

derekbruening commented Jul 5, 2019 •

edited

Loading

i#3700: Add IR for AArch64's SIMD LD1/ST1 instructions #3710

i#3700: Add IR for AArch64's SIMD LD1/ST1 instructions #3710

Conversation

AssadHashmi commented Jun 28, 2019

AssadHashmi commented Jun 28, 2019

derekbruening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn commented Jun 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AssadHashmi commented Jun 28, 2019

AssadHashmi commented Jun 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbruening commented Jul 5, 2019

AssadHashmi commented Jul 5, 2019 • edited Loading

derekbruening commented Jul 5, 2019 • edited Loading

AssadHashmi commented Jul 5, 2019 •

edited

Loading

derekbruening commented Jul 5, 2019 •

edited

Loading