Skip to content

Conversation

@jkressel
Copy link
Contributor

@jkressel jkressel commented Aug 8, 2022

Encodings for ARM v8.2 instructions added.

@lgeek
Copy link
Contributor

lgeek commented Aug 15, 2022

Thanks for the patch.
Please don't change the whitespace in a64.txt as I can't see which lines have been changed and it would break git blame and similar functionality in the future.
Similarly to #7, please double check the argument ordering

Copy link
Contributor

@lgeek lgeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. I've reviewed the patch against ARM DDI 0487B.b, which I believe is the last ARMv8.2 architecture manual. See the inline comments for some requested changes. It would also be useful if you could rebase it on the git HEAD, as I've already merged the AArch32 changes.

I haven't checked the SVE instructions, I think it would be more appropriate to leave them out or move them to a different PR

FMOV_immed 00011110 aa1bbbbb bbb10000 000ccccc, a:type, b:imm8, c:rd
float_cvt_fixed a0011110 bb0ccddd eeeeeeff fffggggg, a:sf, b:type, c:rmode, d:opcode, e:scale, f:rn, g:rd
float_cvt_int a0011110 bb1ccddd 000000ee eeefffff, a:sf, b:type, c:rmode, d:opcode, e:rn, f:rd
float_immed a0b11110 cc1ddddd ddd100ee eeefffff, a:m, b:s, c:ptype, d:imm8, e:imm5, f:rd
Copy link
Contributor

@lgeek lgeek Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see on page C4-321, all the allocated instructions covered by float_immed are already covered by FMOV_immed so I don't think float_immed is needed. Please double check this. In any case, we should only keep one of these 2 definitions.

float_cvt_int a0011110 bb1ccddd 000000ee eeefffff, a:sf, b:type, c:rmode, d:opcode, e:rn, f:rd
float_immed a0b11110 cc1ddddd ddd100ee eeefffff, a:m, b:s, c:ptype, d:imm8, e:imm5, f:rd
float_cvt_fixed a0s11110 bb0ccddd eeeeeeff fffggggg, a:sf, s:s, b:type, c:rmode, d:opcode, e:scale, f:rn, g:rd
float_cvt_int a0s11110 bb1ccddd 000000ee eeefffff, a:sf, s:s, b:type, c:rmode, d:opcode, e:rn, f:rd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above for float_cvt_fixed and float_cvt_int, it doesn't look like S(bit 29)=1 for any of the instructions defined in ARMv8.2 (page C4-311).

float_immed a0b11110 cc1ddddd ddd100ee eeefffff, a:m, b:s, c:ptype, d:imm8, e:imm5, f:rd
float_cvt_fixed a0s11110 bb0ccddd eeeeeeff fffggggg, a:sf, s:s, b:type, c:rmode, d:opcode, e:scale, f:rn, g:rd
float_cvt_int a0s11110 bb1ccddd 000000ee eeefffff, a:sf, s:s, b:type, c:rmode, d:opcode, e:rn, f:rd
simd_three_reg_ext 0ab01110 cc0ddddd 1oooo1ee eeefffff, a:q, b:u, c:size, d:rm, e:rn, f:rd, o:opcode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 'Advanced SIMD three same extra (page C4-296)', right? The name should be updated to better match the documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants