Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i#3044: Add a type to represent aarch64 vectors #5681

Merged
merged 18 commits into from
Oct 21, 2022

Conversation

joshua-warburton
Copy link
Collaborator

Previously in the IR we have represented vectors by a plain H, S, D or Q register combined with a faux-imm added after the last vector to give a hint to its size.

This patch uses the .size field of the opnd struct to include the element size, similar to how partial registers are used for x86. Element Vector registers are differentiated from this by setting the DR_OPND_IS_VECTOR (0x40) bit on the .flag field. This bit is also set by the DR_OPND_IS_EXTEND flag for imms and was chosen as to not extend the size of the flag field while remaining unambiguous.

The new Element Vector operand is printed like z0.b via a check in disassemble_shared.c to set a suffix. Also added are the utility functions opnd_is_element_vector_reg,
opnd_create_reg_element_vector and opnd_get_vector_element_size/

issue: #3044

@AssadHashmi
Copy link
Contributor

Hi @derekbruening, I'm happy with this patch to improve the vector<->element binding for AArch64 SVE instructions. Let us know if you have any objections. We would like this to be the format from SVE onwards. Retro-fitting to v8.0/1/2 SIMD (NEON) may happen later.

@derekbruening
Copy link
Contributor

Hmm, I don't think we've discussed element sizes before: the #3044 discussions were about the mcontext fields.

IIRC, previously the element size was not in the IR at all (for any arch). The size field could be smaller than the register capacity but that meant a sub-reg operand where other bits were left unchanged. Tools that needed the element size (e.g., Dr. Memory) dispatched on opcode.

For this proposal in this PR, my question is: what about an operand that needs both an element size and a sub-register size? IIRC some of the x86 vector operations would leave some bits unchanged and I think the x86 decoder marks those as sub-register (I assume these are not just leaving exactly half unchanged as there we could use the smaller vector register name? or maybe we go w/ the name in the ISA convention b/c other operands are on that full size even when it's half...might have to refresh my memory). Maybe this doesn't happen for SVE? But if we wanted to adopt this for other ISA's it could be an issue there. Not sure what the solution is though other than finding other bits to store the element size.

Maybe taking a step back and seeing whether the sub-reg size is always useful is worthwhile: is it always the bottom bits that are used and the top that are unused? If not then tools have to dispatch on opcode anyway. Are there identical opcodes that differ only by sub-reg size and then a dispatch is not possible.

@derekbruening
Copy link
Contributor

Looking for more opinions: @abhinav92003

@joshua-warburton
Copy link
Collaborator Author

For context, I do not believe partial registers are used anywhere in aarch64.

@derekbruening
Copy link
Contributor

An x86 example:
AMD's VFMADDSS instruction which takes the lower 32 bits of several source XMM registers and adds them. In the destination the top bits are zeroed, but for the sources the top bits are unchanged.
So DR says the sources are XMM registers with size OPSZ_4_of_16

@derekbruening
Copy link
Contributor

If we don't want to increase the size of the passed-by-value opnd_t, can we fit the vector element size in currently-unused-for-register-types bits? The vector element size should be just one of a handful of possibilities. Then we can support both element size and total size. I guess we'd need new accessors to get the element size.

@joshua-warburton
Copy link
Collaborator Author

There doesn't appear to be anything that the element size could be union'd with to fulfil those requirements.

However, there is likely some padding space after the first union, kind(8) + size(8) + union of shorts(16) leaves another 32 bits before the 64-bit aligned second union. Putting an element size in there wouldn't increase the overall size of the struct.

@derekbruening
Copy link
Contributor

For REG_kind for 64-bit pointers, opnd_t.value is only using 16 of its 32 bits (for reg_id_t), so we could add a size for REG_kind there?

@joshua-warburton
Copy link
Collaborator Author

Implemented the above in a new patch.

Previously in the IR we have represented vectors by a
plain H, S, D or Q register combined with a faux-imm
added after the last vector to give a hint to its size.

This patch uses the .size field of the opnd struct to
include the element size, similar to how partial registers
are used for x86. Element Vector registers are differentiated
from this by setting the DR_OPND_IS_VECTOR (0x40) bit on the
.flag field. This bit is also set by the DR_OPND_IS_EXTEND
flag for imms and was chosen as to not extend the size of the
flag field while remaining unambiguous.

The new Element Vector operand is printed like z0.b via a check
in disassemble_shared.c to set a suffix. Also added are the
utility functions opnd_is_element_vector_reg,
opnd_create_reg_element_vector and opnd_get_vector_element_size/

issue: #3044

Change-Id: I0cdb78cc1a13c3db7b6c742fb1d5d5c0e54216ff
Change-Id: I00c846594c0111c9c67741effd62f25c1d21e725
Change-Id: Ie0453e0a5a2e89a1b3cc7e270934f572f4c07e54
Change-Id: Ie3bec227e9478a5c1ce9eff6a64db9628697668d
Change-Id: I017a8716497e33742f092d8b2d55795cd96896fb
Change-Id: Ibb3a8845a286dfb55f1499af06261292ad03891c
Change-Id: I76609940bb34de4c43e821c28e453b0e75936bb2
Change-Id: I9c6f3a710db1cb29324589d2abefdb0c61879936
Change-Id: I7a921aa497e9c53146faff8f7d58cc79159488b1
@joshua-warburton
Copy link
Collaborator Author

run arm tests

@derekbruening
Copy link
Contributor

Turns out there is an issue filed on adding element widths to x86 SIMD operands: #5638

@joshua-warburton
Copy link
Collaborator Author

I believe this is now ready for review, with the suggested change to the storage location @derekbruening @AssadHashmi

Copy link
Contributor

@AssadHashmi AssadHashmi left a comment

Choose a reason for hiding this comment

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

Looks good. Please add 2 tests to verify the edge cases where an instruction has at least one operand which is a:

  • Full SVE vector, i.e. no element size qualification.
  • SIMD/NEON vector (there are SVE instructions which access the lower 128 bits as SIMD/NEON vectors).

Best to avoid memory instruction tests for the above as they will unnecessarily complicate the patch, any simple arithmetic/logic instructions should be fine.

core/ir/disassemble_shared.c Outdated Show resolved Hide resolved
core/ir/instr_inline_api.h Show resolved Hide resolved
core/ir/opnd_api.h Outdated Show resolved Hide resolved
core/ir/opnd_api.h Outdated Show resolved Hide resolved
core/ir/opnd_api.h Outdated Show resolved Hide resolved
core/ir/opnd_shared.c Outdated Show resolved Hide resolved
core/ir/aarch64/instr_create_api.h Outdated Show resolved Hide resolved
core/ir/opnd_api.h Outdated Show resolved Hide resolved
core/ir/opnd_api.h Show resolved Hide resolved
@AssadHashmi
Copy link
Contributor

While reviewing the disassembly changes I noticed an error in reg_names[] for SVE register strings. There's a q3 which should be z3:

"z0", "z1", "z2", "q3", "z4", "z5", "z6", "z7", "z8", "z9",

Please put the fix in this patch, thanks.

Change-Id: I20a70b53f4c50f4b4341fdc9fca1ba2a2131ee8a
Change-Id: I66820e4994492784c947b8adf18ba1516c01b122
Change-Id: I5724b909bb13a5b17b213f41fa1e6b6921ffae76
@joshua-warburton
Copy link
Collaborator Author

I believe this is all the resolvable review comments resolved.

Change-Id: I03ff7febdc0ab6477d0e1a14310ca54a8ce1cb52
Change-Id: Ia536d9a3fc5115915dd17d78c64f9e644750ae47
Change-Id: I0f6be678faf4f0d9f4d7f3c682e636875cbf3635
@joshua-warburton joshua-warburton merged commit 13ff46c into master Oct 21, 2022
@joshua-warburton joshua-warburton deleted the i3044-add-element-vector branch October 21, 2022 10:32
@derekbruening
Copy link
Contributor

Also added are the utility functions opnd_is_element_vector_reg,
opnd_create_reg_element_vector and opnd_get_vector_element_size/

Looks like these new functions were not added to the release notes for new features.

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.

3 participants