JIT/AArch64: Support shifted immediate #7165
Conversation
As pointed out by MikePall in [1], shifted immediate value is supported. See [2]. For example, `add x0, x1, php#4096` would be encoded by DynASM into `add x0, x1, php#1, lsl php#12` directly. In this patch, a helper is added to check whether an immediate value is in the two allowed ranges: (1) 0 to 4095, and (2) LSL php#12 on all the values from the first range. Note that this helper works for add/adds/sub/subs/cmp/cmn instructions. [1] LuaJIT/LuaJIT#718 [2] https://github.com/LuaJIT/LuaJIT/blob/v2.1/dynasm/dasm_arm64.lua#L342 Change-Id: I4870048b9b8e6c429b73a4803af2a3b2d5ec0fbb
ext/opcache/jit/zend_jit_arm64.dasc
Outdated
| @@ -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))); | |||
There was a problem hiding this comment.
Should use either CMP_IMM or drop the constant (no longer used)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have updated the patch. Would you mind taking another look? Thanks! @nikic
There was a problem hiding this comment.
Could you help to take another round of review on the latest commit when you have spare time? Thanks. @nikic
| | cmp reg, #val | ||
| || } else if (((int64_t)(val)) < 0 && ((int64_t)(val)) >= -CMP_IMM) { | ||
| || } else if (arm64_may_encode_imm12((int64_t)(-val))) { |
There was a problem hiding this comment.
Did you test this?
In any case, it would be great to add test(s) for edge cases.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Two test cases are added, and in my local testing, immediate values in legitimate ranges can be encoded as the imm field as expected.
- for
cmpandcmninstructions, I only testedCMP_64_WITH_CONST. Note: I tested that negative values can be encoded into theimmfield focmninstruction. - for ADD and SUB, I only tested
addsandsubswith macroADD_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.
Macros CMP_IMM and ADD_SUB_IMM are deprecated and instead we use this helper to guard the immediate encoding. Add two 64-bit only test cases, since 64-bit integers are used and tested inside. Change-Id: I0b42d4617b40372e2f4ce5b6ad31a4ddb7d89e49
As pointed out by MikePall in [1], shifted immediate value is supported.
See [2]. For example,
add x0, x1, #4096would be encoded by DynASMinto
add x0, x1, #1, lsl #12directly.In this patch, a helper is added to check whether an immediate value is
in the two allowed ranges: (1) 0 to 4095, and (2)
LSL #12on all thevalues from the first range.
Note that this helper works for add/adds/sub/subs/cmp/cmn instructions.
[1] LuaJIT/LuaJIT#718
[2]
https://github.com/LuaJIT/LuaJIT/blob/v2.1/dynasm/dasm_arm64.lua#L342
Test: all ~4k .phpt test cases under
tests/ Zend/tests/ ext/opcache/tests/jit/can pass for Linux JIT/arm64.Note that in total 8 JIT variants are tested, covering ZTS/nonZTS, HYBRID/VM, and functional/tracing JIT.
Change-Id: I4870048b9b8e6c429b73a4803af2a3b2d5ec0fbb