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#2626 fp/simd encode: Add support for most FP Neon instructions. #2951

Merged
merged 4 commits into from
Apr 25, 2018

Conversation

fhahn
Copy link
Contributor

@fhahn fhahn commented Apr 24, 2018

This patch adds encoding & decoding support for most arithmetic FP Neon
instructions. The structure of the generated macros and tests follows
the style of #2896.

codec.txt contains comments if not all encodings of an instruction are
supported yet. Also missing are the Arm v8.3-a FCADD and FCMLA
instructions, because their rotation arguments require a bit of special
handling to get nice macros.

Issue #2626

This patch adds encoding & decoding support for most arithmetic FP Neon
instructions. The structure of the generated macros and tests follows
the style of #2896.

codec.txt contains comments if not all encodings of an instruction are
supported yet. Also missing are the Arm v8.3-a FCADD and FCMLA
instructions, because their rotation arguments require a bit of special
handling to get nice macros.

Issue #2626

Change-Id: I8c3a8ac3e03d9b466c1872ed5382639b746092b2
@fhahn fhahn requested review from derekbruening and egrimley April 24, 2018 09:15
@fhahn
Copy link
Contributor Author

fhahn commented Apr 24, 2018

@derekbruening I could also split this one into smaller patches, if that would make things easier to review. Most of the changes are only mechanical though

@derekbruening
Copy link
Contributor

Ideally I would go look in the manual and verify (at least spot-check a few of) the binary encodings for a thorough review -- has there been a cross-checking versus an existing decoder/encoder?

00011110xx1xxxxx000110xxxxxxxxxx fdiv float_reg0 : float_reg5 float_reg16

# FMADD
00011111xx0xxxxx0xxxxxxxxxxxxxxx fmadd float_reg0 : float_reg5 float_reg16 float_reg10
Copy link
Contributor

Choose a reason for hiding this comment

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

I spot-checked just FMADD and it seems the bit after the 5 1's (bit 23) can be 0 and not x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the A64 ISA XML [1], bits 22 and 23 of FMADD (and other FP instructions) encode the element type (00 = Single, 01 = double, 11 = Half). xx here means it is set by (at least) one of the operands, in that case that case float_reg0, float_reg5, float_reg16, float_reg10. The actual encoding of the type is done here.

[1] https://developer.arm.com/products/architecture/a-profile/exploration-tools

Copy link
Contributor

Choose a reason for hiding this comment

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

In the ARM Architecture Reference Manual for ARMv8, maybe old since from 2013 ARM DDI 0487A.a (ID090413), the FMADD entry in C6.3.87 on page C6-924 says:

31 30 29 28 27 26 25 24 23 22 21 20    16 15 14    10 9    5 4    0
 0  0  0  1  1  1  1  1  0  x  0       Rm  0    Ra      Rn     Rd
                         type o1          o0

I.e., it is saying that the top bit of the type is always 0 -- meaning the half type is not supported. Yet encode_opnd_float_reg() will accept half.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes. The half encodings were added in Arm v8.2. That leads us to an interesting question: How should we deal with that? I also realized that there is no comment indicating that the half version is 8.2.

Copy link
Contributor

Choose a reason for hiding this comment

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

For x86 we do not treat processor revisions as first-class in the encoder/decoder: it will accept a newer encoding when running on an older processor (and deal with it for execution only when SIGILL is raised) so it doesn't seem so bad to keep it how you have it, with perhaps as you say a comment in encode_opnd_float_reg() saying that a half type on 8.1 is illegal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great thanks for having a look. Will do that tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Xref the drcpusim tool [1] which has its own accounting of which encodings are valid on which processor because the DR ISA doesn't tell it. Its use case is an app that contains different assembly sequences for different architectures (such as optimized multimedia code) and wants to test that it's using only legal instructions on some legacy processor. (Would such a tool be useful for ARM as well or does ARM already provide such a tool?)

[1] http://dynamorio.org/docs/page_drcpusim.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think Arm currently provides such a tool, but I suppose it could be useful once more hardware implementing different version of Armv8 becomes available.

We discussed providing some kind of API to check which version of the ISA supports an encoding internally. I've created #2960 to discuss the details.

@fhahn
Copy link
Contributor Author

fhahn commented Apr 24, 2018

The dis-a64 test should verify that binutils and our assembler/disassembler agree on the test cases in suite/tests/api/dis-a64.txt. The line should contain :

Encoding (generated by binutils in this patch) : assembly string : expected DynamoRIO IR

For each line, dis-a64 should read the encoding, disassemble it, compare the resulting IR to the expected and assemble the IR and check if it matches the original encoding. I think that's pretty good, but of course it would not catch our disassembler accepting invalid encodings for example.

@fhahn
Copy link
Contributor Author

fhahn commented Apr 24, 2018

That said, I think we could improve on the negative testing, e.g. checking we fail encoding invalid IR.

Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

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

Approving in anticipation of comment(s) on the half type.

@fhahn fhahn merged commit 75da1c2 into master Apr 25, 2018
@fhahn fhahn deleted the i2626-neon-fp-new branch April 25, 2018 10:25
@fhahn
Copy link
Contributor Author

fhahn commented Apr 25, 2018

Thank you very much Derek for your quick feedback!

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