Closed FinnWilkinson closed 4 months ago
Looking into this a bit further, it seems to originate from the AArch64GenCSMappingInsnOp.inc
file where all SME based instructions with an index have CS_OP_MEM
.
In capstone's LLVM, llvm/utils/TableGen/PrinterCapstone.cpp
the function getCSOperandType
contains
if (TargetName.equals("AArch64") && OperandType != "CS_OP_MEM") {
// The definitions of AArch64 are so broken, when it comes to memory
// operands, that we just search for the op name enclosed in [].
if (Regex("\\[.*\\$" + OpName.str() + ".*]").match(CGI->AsmString))
return OperandType += " | CS_OP_MEM";
}
Which would explain the above issues with LD1W
and ST1W
whereby p2
is seen as a memory operand (The open bracket [
is part of za0h
's index, the close bracket ]
is part of the actual memory operand
I think it also explains the issue with PSEL
as all predicate registers are recognised correctly but p3
's index is seen as a memory operand rather than its index.
Looking at other SME instructions in AArch64GenCSMappingInsnOp.inc
, instruction opcode AArch64_ADD_VG2_M2Z2Z_D
could potentially present a similar issue to PSEL where ZA's index is seen as a memory operand rather than an index --- again this could be explained by the above regex
{ /* AArch64_ADD_VG2_M2Z2Z_D (1265) - AArch64_INS_ADD - add $ZAd[$Rv, $imm3, vgx2], $Zn, $Zm */
{
{ CS_OP_REG, CS_AC_WRITE, { CS_DATA_TYPE_Untyped, CS_DATA_TYPE_LAST } }, /* ZAd */
{ CS_OP_REG, CS_AC_READ, { CS_DATA_TYPE_Untyped, CS_DATA_TYPE_LAST } }, /* _ZAd */
{ CS_OP_REG | CS_OP_MEM, CS_AC_INVALID, { CS_DATA_TYPE_i32, CS_DATA_TYPE_LAST } }, /* Rv */
{ CS_OP_IMM | CS_OP_MEM, CS_AC_INVALID, { CS_DATA_TYPE_i32, CS_DATA_TYPE_LAST } }, /* imm3 */
{ CS_OP_REG, CS_AC_READ, { CS_DATA_TYPE_Untyped, CS_DATA_TYPE_LAST } }, /* Zn */
{ CS_OP_REG, CS_AC_READ, { CS_DATA_TYPE_Untyped, CS_DATA_TYPE_LAST } }, /* Zm */
{ 0 }
}},
I'm not 100% sure on how to implement a fix for this, but for the regex above; all AArch64 memory operands will be preceeded by ,
(i.e. , [x6]
) and so the regex needs to accout for this. If [...]
is not preceeded by a comma and a space then we know it to be an index.
Additionally, said regex should check that there is no ]
before the OpName.str()
to bulletproof it from issues seen with PSEL. It may also require another aarch64_op_type
and/or member of the union in cs_aarch64_op
as PSEL does not nicely fit the criteria for aarch64_op_sme
but still requires a register to have an index with a base register and immidiate offset.
As far as I can tell, other AArch64 instructions do not seem to be affected, only SME/SME2.
Thanks a lot for the detailed issue! Currently I am updating to LLVM 18 and will add your fix after it.
Couldn't find the time yet to address the other issues in https://github.com/capstone-engine/capstone/issues/2196 unfortunately. But will address them and other bugs after the LLVM 18 update. Since it adds new AArch64 instructions anyways.
Great, thank you for the update!
@FinnWilkinson I am currently at this. Thanks for spotting the faulty regex. It is fixed now.
Regarding the representation of SME operands.
What do you think about having two types of SME operands. Once an indexed matrix (which is the struct currently called aarch64_op_sme
) and an "indexed" predicate reg op?
Something like:
typedef struct {
aarch64_sme_op_type type; ///< AArch64_SME_OP_TILE, AArch64_SME_OP_TILE_VEC
aarch64_reg tile; ///< Matrix tile register
aarch64_reg slice_reg; ///< slice index reg
// ...
} aarch64_op_sme_matrix;
typedef struct {
// Some type field
aarch64_reg pred_reg;
aarch64_reg vec_select;
int32_t index; ///< Potentially also a union of other ops which can be used here.
} aarch64_op_sme_pred;
typedef struct {
// Some type field
union {
aarch64_op_sme_matrix matrix;
aarch64_op_sme_pred pred;
}
} aarch64_sme_op;
I am a little hesitant to add more operands. But the SME ops don't really fit in the previous ones. So I'd really appreciate your comment on this, since you seem to work with this AArch64 extension a lot.
As far as I can tell, other AArch64 instructions do not seem to be affected, only SME/SME2.
Yeah, SME/SME2 is really not that well defined in LLVM, unfortunately. So they need all kind of hacky solutions.
This is now the resulting output:
cstool -d aarch64 c0089fe04131a2e067447125
0 c0 08 9f e0 ld1w {za0h.s[w12, 0]}, p2/z, [x6]
ID: 474 (ld1w)
Is alias: 1466 (ld1w) with ALIAS operand set
op_count: 3
operands[0].type: SME_MATRIX
operands[0].sme.type: 2
operands[0].sme.mx.tile: za0.s
operands[0].sme.mx.slice_reg: w12
operands[0].sme.mx.slice_offset: 0
operands[0].sme.mx.is_vertical: false
operands[0].access: WRITE
Vector Arrangement Specifier: 0x20
operands[1].type: SME_PRED
operands[1].sme.pred.reg: p2
operands[1].access: READ
operands[2].type: MEM
operands[2].mem.base: REG = x6
operands[2].access: READ
Registers read: za0.s w12 p2 x6
Groups: HasSME
4 41 31 a2 e0 st1w {za0h.s[w13, 1]}, p4, [x10, x2, lsl #2]
ID: 1099 (st1w)
op_count: 3
operands[0].type: SME_MATRIX
operands[0].sme.type: 2
operands[0].sme.mx.tile: za0.s
operands[0].sme.mx.slice_reg: w13
operands[0].sme.mx.slice_offset: 1
operands[0].sme.mx.is_vertical: false
operands[0].access: READ
Vector Arrangement Specifier: 0x20
operands[1].type: SME_PRED
operands[1].sme.pred.reg: p4
operands[1].access: READ
operands[2].type: MEM
operands[2].mem.base: REG = x10
operands[2].mem.index: REG = x2
operands[2].access: WRITE
Shift: type = 1, value = 2
Registers read: za0.s w13 p4 x10 x2
Groups: HasSME
8 67 44 71 25 psel p7, p1, p3.s[w13, 1]
ID: 785 (psel)
op_count: 3
operands[0].type: SME_PRED
operands[0].sme.pred.reg: p7
operands[0].access: WRITE
operands[1].type: SME_PRED
operands[1].sme.pred.reg: p1
operands[1].access: READ
operands[2].type: SME_PRED
operands[2].sme.pred.reg: p3
operands[2].sme.pred.vec_select: w13
operands[2].sme.pred.index: 1
operands[2].access: READ
Vector Arrangement Specifier: 0x20
Registers read: p7 p1 p3 w13
Groups: HasSVE2p1_or_HasSME
Hi @Rot127 , sorry for the delayed reply I've only just seen the newest comments. This generallt all looks good to me, but I have some concerns about predicates in HasSME
instructions. I assume only predicates in instructions with the group HasSME
are of type sme.pred.reg
? Even so, I think it is slightly confusing. For example, in psel
there is no reference to an SME operand, yet all predicates are now sme.pred.reg
even when the predicate is not indexed. A possible solution could be to add a variable to cs_aarch_op
called index
which any operand can have:
typedef struct aarch64_op_index {
aarch64_reg base;
int32_t imm_offset;
} aarch64_op_index;
This could also be used by NEON, SVE, and predicate registers and remove the need for vector_index
. I'm sure this change in non-trivial though (and adds to current logic which isn't ideal), but could tidy things up a bit?
Additionally, just to ensure functional correctness, what do the following instructions get disassembled as?
fmopa za2.s, p0/m, p2/m, z3.s, z4.s
==== 40208180smstart
==== 7f4703d5zero {za0.s, za2.s}
==== 550008c0
za0.h
is a valid alias for za0.s
and za2.s
)A possible solution could be to add a variable to cs_aarch_op called index which any operand can have:
The thing is that these indices differ quite a lot (the matrix indices can have immediate ranges. And additionally the terminology differs between the members of the indices structs (slice
, vector select
etc.)). Would you be ok with:
SME
operandIndex
struct as suggested with: index_type
(None
, VectorPredicate
, Matrix
or something) + a union with the different indices structs.I'd like to have the terminology of the struct fields relatively close to the ISA, if possible Hence not just one index struct, but multiple.
Generally though I would follow your advice. I only interact with AArch64 in the Capstone context. And because you seem to use it heavily in practice, I would follow your design in the end.
I'm sure this change in non-trivial though (and adds to current logic which isn't ideal), but could tidy things up a bit?
As long as we don't change it after the v6
release, it's ok.
v6
will be huge, and although it is annoying to have these API changes here in next
, it is important that it is done right before v6
. And next
is still a development branch in the end.
Additionally, just to ensure functional correctness, what do the following instructions get disassembled as?
Thanks for the test cases! As it looks like the alias versions are still miss some operands. Please let me know any other errors.
If you have more test cases, please let me know. Those operands are pretty complex.
I think the current implementation of SME operands (non predicates) works well, so I'd be opposed to changing from this. How about having a new aarch64_op_pred
type for all predicates (not just HasSME
)? This leaves vector and SME indexing as is, and would make indexed predicates a tad easier IMO without too much overhead for normal, non-indexed, predicate registers.
For the test case outputs:
smstart
ALIAS version printed the same as REAL then that would be helpfulzero
ALIAS is missing the register, and REAL looks to select the incorrect register (this isn't a valid alias as far as I know)I'll compile some more test cases now and put them here, but I need to formulate the hex first.
I'll compile some more test cases now and put them here, but I need to formulate the hex first.
This would be great. Thank you!
How about having a new aarch64_op_pred type for all predicates (not just HasSME)?
Ok, sounds good to me. To summarize:
Add new operand type aarch64_op_pred
and aarch64_op_index
.
typedef struct aarch64_op_index {
aarch64_reg base;
int32_t imm_offset;
} aarch64_op_index;
typedef struct aarch64_op_pred {
aarch64_reg pred;
aarch64_op_index index;
} aarch64_op_pred;
sme.pred
, get converted to the new aarch64_op_pred
format.sme
untouched (meaning: in https://github.com/capstone-engine/capstone/pull/2298 change it back to the previous sme
operand, instead of sme.mx
).Will aarch64_op_index
be used for anything else? If not couldn't we just do
typedef struct aarch64_op_pred {
aarch64_reg pred;
aarch64_reg vec_select;
int32_t im_offset;
} aarch64_op_pred;
?
And for sme
instructions yeah, just removing the .mx
part to yield (for example)
...
operands[0].type: SME
operands[0].sme.type: 2
operands[0].sme.tile: za0.s
operands[0].sme.slice_reg: w12
operands[0].sme.slice_offset: 0
operands[0].sme.is_vertical: false
...
seems good to me.
Will aarch64_op_index be used for anything else? If not couldn't we just do
Ah, well. Yes. Let's just do it as you said. I wasn't paying attention and thought about the SME matrix as well. But let's just hope ARM will not introduce more extensions with this index pattern.
But let's just hope ARM will not introduce more extensions with this index pattern.
Yes, lets!
Here are some more complex assembly tests that would be good to validate as working. At the moment this is about as complex as SVE and SME gets (hex may be the wrong way round):
sdot za.s[w11, 2, vgx4], {z0.h-z3.h}, z5.h[2]
==== c155f802movaz {z4.d-z7.d}, za.d[w8, 5, vgx4]
==== c0060ea4luti2 {z0.s-z3.s}, zt0, z4[1]
==== c08da080fmla za.h[w9, 0, vgx4], {z8.h-z11.h}, z0.h[0]
==== c110b100fmlal za.s[w10, 2:3, vgx4], {z0.h-z3.h}, z11.h[1]
==== c19bd005Change is done. Also fixed, that the predicate regs were not added to the register written
list.
If smstart ALIAS version printed the same as REAL then that would be helpful
Sorry, late night working. Got confused with my own implementation.
This behavior (alias not printing the operands) is on purpose. cstool
is supposed to print only the details which
are part of the asm text (this is forced by the design of Capstone).
The real operand set can always be accessed, either by adding the -r
flag to cstool
or by enabling the
CS_OPT_DETAIL_REAL
option.
The reason is: to retrieve both detail sets, an instruction must be disassembled twice. And this is a choice only the user should make. So we don't double the runtime by default.
zero ALIAS is missing the register, and REAL looks to select the incorrect register (this isn't a valid alias as far as I know)
Fix this later this week.
The new test cases
Output is below. Note the register lists. I didn't add a new operand type for those. If you say it would be a huge improvement to have them in a separated struct, we can talk about the implementation. Otherwise, I'd like to let the user deduct the registers in between the first and last register. Just to keep it somewhat simple.
Same for the vgx4
indicators. They are not stored in the details
, because it can be deducted by checking the list
members.
Change is done. Also fixed, that the predicate regs were not added to the register written list.
I think this looks great now. Thanks!
Sorry, late night working. Got confused with my own implementation. This behavior (alias not printing the operands) is on purpose. cstool is supposed to print only the details which are part of the asm text (this is forced by the design of Capstone).
Understood!
Note the register lists. I didn't add a new operand type for those.
Given you can have strided vector register lists as well as consecutive (although I'm struggling to find an example currently), I think it would be beneficial to print them individually for both clarity and for using Capstone as a tool inside other projects. Rather than a new structure though, could we not print them as seperate operands?
i,.e. luti2 { z0.s - z3.s }, zt0, z4[1]
would be:
8 c0 8d a0 80 luti2 { z0.s - z3.s }, zt0, z4[1]
ID: 710 (luti2)
op_count: 6
operands[0].type: REG = z0
operands[0].is_list_member: true
operands[0].access: WRITE
Vector Arrangement Specifier: 0x20
operands[1].type: REG = z1
operands[1].is_list_member: true
operands[1].access: WRITE
Vector Arrangement Specifier: 0x20
operands[2].type: REG = z2
operands[2].is_list_member: true
operands[2].access: WRITE
Vector Arrangement Specifier: 0x20
operands[3].type: REG = z3
operands[3].is_list_member: true
operands[3].access: WRITE
Vector Arrangement Specifier: 0x20
operands[4].type: SME_MATRIX
operands[4].sme.type: 1
operands[4].sme.tile: zt0
operands[4].access: READ
operands[5].type: REG = z4
operands[6].access: READ
Vector Index: 1
Registers read: zt0 z4
Registers modified: z0 z1 z2 z3
Groups: HasSME2
The SME2 spec also states that although { z0.s - z3.s }
is preferred disassembly, there must also be support for { z0.s, z1.s, z2.s, z3.s }
. We could change to this other disassembly format to make list operands clearer, but I prefer the simpler format (i.e. keep it as it is now).
Same for the vgx4 indicators.
I agree with not printing vgx4
etc, as the vg
syntax is optional disassembly as per the spec and doesn't really add much to the understanding. Having it printed in the disassembly is enough IMO.
As an aside, should zt0
be classed as a normal register rahter than SME_MATRIX
? Although only accessible in Streaming SVE mode, it is a seperate register to ZA
. And from what I can tell from the spec, it is not indexable (yet...). If you think theres a good reason to keep zt0
as is though then I'm still ok with how its currently displayed.
I've also just noticed that SMSTART
currently has Registers modified: nzcv
which is false (as far as I can tell from the spec). Only PSTATE
is updated. Similar is true for SMSTOP
.
The smstart
is fixed. Also here the zero
operands:
Real:
8 55 00 08 c0 zero {za0.h}
ID: 1384 (zero)
Is alias: 1470 (zero) with REAL operand set
op_count: 5
operands[0].type: SME_MATRIX
operands[0].sme.type: 1
operands[0].sme.tile: za0.d
operands[0].access: READ
Vector Arrangement Specifier: 0x40
operands[1].type: SME_MATRIX
operands[1].sme.type: 1
operands[1].sme.tile: za2.d
operands[1].access: READ
Vector Arrangement Specifier: 0x40
operands[2].type: SME_MATRIX
operands[2].sme.type: 1
operands[2].sme.tile: za4.d
operands[2].access: READ
Vector Arrangement Specifier: 0x40
operands[3].type: SME_MATRIX
operands[3].sme.type: 1
operands[3].sme.tile: za6.d
operands[3].access: READ
Vector Arrangement Specifier: 0x40
Registers read: za0.d za2.d za4.d za6.d
Groups: HasSME
Alias:
8 55 00 08 c0 zero {za0.h}
ID: 1384 (zero)
Is alias: 1470 (zero) with ALIAS operand set
op_count: 1
operands[0].type: SME_MATRIX
operands[0].sme.type: 1
operands[0].sme.tile: za0.h
Vector Arrangement Specifier: 0x10
Registers read: za0.h
Groups: HasSME
Register lists are now as you suggested.
Here the output of all instructions:
I'll add all the instructions from here as tests now ans start fuzzing afterwards. If you still find something, please let me know.
Thanks for helping out btw. It is difficult to get everything right in detail if you have to handle 3+ architectures with extensions. It's easy to miss things if one doesn't work with them all the time.
All the above look great, thanks very mcuh for all your hard work on this! I'm looking forward to it being merged into next
.
If I spot anything further I'll be sure to let you know / open new issues.
Working on the latest
next
branch, disassembling a range of SME instructions yields incorrect disassembly information, often with additional memory operands being defined.It seems to be an issue with the pattern matching of
[...]
. A similar issue occurred when implementing PR #1907 and was fixed in PR #1925. I'm not sure how different the backend is now after the auto-sync update, but perhaps a similar fix can be implemented from this?Listed below are some examples:
Issue - Operand 2
p3.s
's index is identified as an additional memory operand.Issue - Operand
p4
is mistaken for a memory operand andx10
as its register offset. Also, operands[0] should not have an access specifier, and operands[2] should not have a vector indexIssue - Same as above.