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

ARM 32-bit port: add support for ARMv8 in 32-bit mode, a.k.a. AArch32 #1486

Merged
merged 2 commits into from Dec 28, 2017

Conversation

Projects
None yet
2 participants
@xavierleroy
Copy link
Contributor

xavierleroy commented Nov 18, 2017

ARMv8, the latest and greatest ARM architecture, has two modes: AArch64 in 64 bits and AArch32 in 32 bits. AArch32 is almost identical to ARMv7, the previous, 32-bit-only incarnation of ARM. However, some conditional instructions are marked as "deprecated" and cause an assembler warning. Quoting from an ARMv8 manual:

In conjunction with the reduction of conditionality in the A64 instruction set, and to facilitate higher performance implementations of the architecture in the future, ARMv8 deprecates some uses of the T32 IT instruction. All uses of IT that apply to other than a single subsequent 16-bit instruction from a restricted set are deprecated, as are explicit references to R15 (i.e. PC) within that single 16-bit instruction. This permits the non-deprecated forms of IT and subsequent instruction to be treated by the processor as a single 32-bit conditional instruction. The restricted set of 16-bit instructions which are not deprecated when used in conjunction with IT are as follows: [...]

This pull request updates the ARM 32 bit port of OCaml as follows:

  • Add a new architecture variant Archi.ARMv8
  • Recognize it in configure
  • Adapt code generation for condition operators to not use ITE blocks on ARMv8/Thumb2 and use IT blocks from the restricted set of instructions that are not deprecated.

Side note: the ARM cases in configure are a bit of a mess but this will be dealt with as part of a future cleanup of configure.

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented Nov 18, 2017

PS. This was tested on an ARMv7 machine, first with default configuration, then by forcing armv8 at configure time.

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented Dec 3, 2017

PPS. A formal yes/no review is requested. @mshinwell or @chambart perhaps? (both know their way around exotic architectures).

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Dec 6, 2017

I will review this.

@mshinwell
Copy link
Contributor

mshinwell left a comment

OK for merging after minor comments addressed.

` it {emit_string compelse}\n`;
` mov{emit_string compelse} {emit_reg temp}, #0\n`;
if temp.loc = rd.loc then 4 else begin
` movs {emit_reg rd}, {emit_reg temp}\n`; 5

This comment has been minimized.

@mshinwell

mshinwell Dec 6, 2017

Contributor

I don't understand why this needs to be movs. I thought the condition flags would already be in the correct state.

This comment has been minimized.

@xavierleroy

xavierleroy Dec 28, 2017

Author Contributor

In Thumb-2 mode, movs has a 16-bit encoding while mov is generally 32 bit encoded. So, when flags do not matter, as in the case here, movs is preferred.

end else begin
(* T32 mode in ARMv8 deprecates general ITE blocks
and favors IT blocks containing only one 16-bit instruction.
mov <reg>, #<imm> is 16 bits if <reg> is R0...R7 and <imm> is 0 or 1. *)

This comment has been minimized.

@mshinwell

mshinwell Dec 6, 2017

Contributor

I found this comment a bit misleading -- I know it says "if" not "iff", but it would probably be clearer if it said the actual condition on <imm>, which is that it fits in 8 bits.

This comment has been minimized.

@xavierleroy

xavierleroy Dec 28, 2017

Author Contributor

Right, I put the actual condition.

` ite {emit_string compthen}\n`;
` mov{emit_string compthen} {emit_reg i.res.(0)}, #1\n`;
` mov{emit_string compelse} {emit_reg i.res.(0)}, #0\n`; 4
emit_set_condition cmp i.res.(0) + 1

This comment has been minimized.

@mshinwell

mshinwell Dec 6, 2017

Contributor

I found the addition on this line (and the one below) confusing at first. Maybe "1 + emit_set_condition ..." would be clearer?

This comment has been minimized.

@xavierleroy

xavierleroy Dec 28, 2017

Author Contributor

I hesitated between the two. If you find 1 + ... clearer, let's go for it.

| EABI, _ -> ARMv4, Soft, false
| EABI_HF, "armv6" -> ARMv6, VFPv2, false
| EABI_HF, "armv8" -> ARMv8, VFPv3, true

This comment has been minimized.

@mshinwell

mshinwell Dec 6, 2017

Contributor

As a note, I checked about VFPv3-D16 on ARMv8: it turns out such a thing doesn't exist. FP implementations on v8 must have 32 registers.
I wonder why our ARMv7 default is for VFPv3-D16.

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented Dec 28, 2017

Thanks for the review. Suggestions addressed in latest commit. One more round of precheck CI and I'll merge.

xavierleroy added some commits Nov 17, 2017

ARM 32-bit port: add support for ARMv8 in 32-bit mode, a.k.a. AArch32
For this platform, avoid ITE conditional instruction blocks and use
simpler IT blocks instead.
ARM 32-bit port: add support for ARMv8 in 32-bit mode, continued
- Address comments from review of GPR#1486.
- Update Changes.

@xavierleroy xavierleroy force-pushed the xavierleroy:aarch32 branch from 5ef7b27 to 2b35b81 Dec 28, 2017

@xavierleroy xavierleroy merged commit e71f102 into ocaml:trunk Dec 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@xavierleroy xavierleroy deleted the xavierleroy:aarch32 branch Jun 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.