Skip to content

JIT/AArch64: Support shifted immediate #7165

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 23, 2021
Merged

JIT/AArch64: Support shifted immediate #7165

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1bb0d3
JIT/AArch64: Support shifted immediate
shqking Jun 15, 2021
4b57997
Deprecatd CMP_IMM/ADD_SUB_IMM and add test cases
shqking Jun 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions ext/opcache/jit/zend_jit_arm64.dasc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ typedef struct TLVDescriptor TLVDescriptor;

#define IS_SIGNED_32BIT(val) ((((intptr_t)(val)) <= 0x7fffffff) && (((intptr_t)(val)) >= (-2147483647 - 1)))

/* Encoding of immediate. TODO: shift mode may be supported in the near future. */
/* Encoding of immediate. */
#define MAX_IMM12 0xfff // maximum value for imm12
#define MAX_IMM16 0xffff // maximum value for imm16
#define CMP_IMM MAX_IMM12 // cmp insn
Expand Down Expand Up @@ -172,6 +172,11 @@ static bool arm64_may_use_adrp(const void *addr)
return 0;
}

static bool arm64_may_encode_imm12(const int64_t val)
{
return (val >= 0 && (val < (1<<12) || !(val & 0xffffffffff000fff)));
Copy link
Member

@nikic nikic Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Should use either CMP_IMM or drop the constant (no longer used)?

Copy link
Contributor Author

@shqking shqking Jun 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your comment.

I plan to change val < (1<<12) to val <= MAX_IMM12, and deprecate CMP_IMM since it's no longer used as you said.

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 have updated the patch. Would you mind taking another look? Thanks! @nikic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you help to take another round of review on the latest commit when you have spare time? Thanks. @nikic

}

/* Determine whether an immediate value can be encoded as the immediate operand of logical instructions. */
static bool logical_immediate_p(uint64_t value, uint32_t reg_size)
{
Expand Down Expand Up @@ -364,9 +369,9 @@ static bool logical_immediate_p(uint64_t value, uint32_t reg_size)
|.macro CMP_32_WITH_CONST, reg, val, tmp_reg
|| if (val == 0) {
| cmp reg, wzr
|| } else if (((int32_t)(val)) > 0 && ((int32_t)(val)) <= CMP_IMM) {
|| } else if (arm64_may_encode_imm12((int64_t)(val))) {
| cmp reg, #val
|| } else if (((int32_t)(val)) < 0 && ((int32_t)(val)) >= -CMP_IMM) {
|| } else if (arm64_may_encode_imm12((int64_t)(-val))) {
| cmn reg, #-val
|| } else {
| LOAD_32BIT_VAL tmp_reg, val
Expand All @@ -377,9 +382,9 @@ static bool logical_immediate_p(uint64_t value, uint32_t reg_size)
|.macro CMP_64_WITH_CONST_32, reg, val, tmp_reg
|| if (val == 0) {
| cmp reg, xzr
|| } else if (((int32_t)(val)) > 0 && ((int32_t)(val)) <= CMP_IMM) {
|| } else if (arm64_may_encode_imm12((int64_t)(val))) {
| cmp reg, #val
|| } else if (((int32_t)(val)) < 0 && ((int32_t)(val)) >= -CMP_IMM) {
|| } else if (arm64_may_encode_imm12((int64_t)(-val))) {
| cmn reg, #-val
|| } else {
| LOAD_32BIT_VAL tmp_reg, val
Expand All @@ -390,9 +395,9 @@ static bool logical_immediate_p(uint64_t value, uint32_t reg_size)
|.macro CMP_64_WITH_CONST, reg, val, tmp_reg
|| if (val == 0) {
| cmp reg, xzr
|| } else if (((int64_t)(val)) > 0 && ((int64_t)(val)) <= CMP_IMM) {
|| } else if (arm64_may_encode_imm12((int64_t)(val))) {
| cmp reg, #val
|| } else if (((int64_t)(val)) < 0 && ((int64_t)(val)) >= -CMP_IMM) {
|| } else if (arm64_may_encode_imm12((int64_t)(-val))) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you test this?
In any case, it would be great to add test(s) for edge cases.

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 have run ~4k .phpt test cases under "Zend/tests/, tests/ ext/opcache/tests/jit/", and didn't find any failure.

Good suggestion! I will add several new test cases soon. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two test cases are added, and in my local testing, immediate values in legitimate ranges can be encoded as the imm field as expected.

  1. for cmp and cmn instructions, I only tested CMP_64_WITH_CONST. Note: I tested that negative values can be encoded into the imm field fo cmn instruction.
  2. for ADD and SUB, I only tested adds and subs with macro ADD_SUB_64_WITH_CONST.

I currently failed to construct test cases to use CMP_32_WITH_CONST, CMP_64_WITH_CONST_32, ADD_SUB_32_WITH_CONST, ADD_SUB_64_WITH_CONST_32, but I think arm64_may_encode_imm12 would work for them as well since these 4 macros share the same encoding logic of imm12 with CMP_64_WITH_CONST and ADD_SUB_64_WITH_CONST.

| cmn reg, #-val
|| } else {
| LOAD_64BIT_VAL tmp_reg, val
Expand All @@ -406,7 +411,7 @@ static bool logical_immediate_p(uint64_t value, uint32_t reg_size)
|.macro ADD_SUB_32_WITH_CONST, add_sub_ins, dst_reg, src_reg1, val, tmp_reg
|| if (val == 0) {
| add_sub_ins dst_reg, src_reg1, wzr
|| } else if (((int32_t)(val)) > 0 && ((int32_t)(val)) <= ADD_SUB_IMM) {
|| } else if (arm64_may_encode_imm12((int64_t)(val))) {
| add_sub_ins dst_reg, src_reg1, #val
|| } else {
| LOAD_32BIT_VAL tmp_reg, val
Expand All @@ -417,7 +422,7 @@ static bool logical_immediate_p(uint64_t value, uint32_t reg_size)
|.macro ADD_SUB_64_WITH_CONST_32, add_sub_ins, dst_reg, src_reg1, val, tmp_reg
|| if (val == 0) {
| add_sub_ins dst_reg, src_reg1, xzr
|| } else if (((int32_t)(val)) > 0 && ((int32_t)(val)) <= ADD_SUB_IMM) {
|| } else if (arm64_may_encode_imm12((int64_t)(val))) {
| add_sub_ins dst_reg, src_reg1, #val
|| } else {
| LOAD_32BIT_VAL tmp_reg, val
Expand All @@ -428,7 +433,7 @@ static bool logical_immediate_p(uint64_t value, uint32_t reg_size)
|.macro ADD_SUB_64_WITH_CONST, add_sub_ins, dst_reg, src_reg1, val, tmp_reg
|| if (val == 0) {
| add_sub_ins dst_reg, src_reg1, xzr
|| } else if (((int64_t)(val)) > 0 && ((int64_t)(val)) <= ADD_SUB_IMM) {
|| } else if (arm64_may_encode_imm12((int64_t)(val))) {
| add_sub_ins dst_reg, src_reg1, #val
|| } else {
| LOAD_64BIT_VAL tmp_reg, val
Expand Down