[all]: Rework Neon load/store and load/store pair #804
Conversation
|
Following #749 (comment) |
herd/AArch64Sem.ml
Outdated
| | I_STUR_SIMD(var,r1,rA,k) -> | ||
| let access_size = tr_simd_variant var and | ||
| k = K (match k with Some k -> k | None -> 0) in | ||
| simd_str access_size rA r1 k S_NOEXT ii | ||
| k = match k with Some k -> k | None -> 0 in |
There was a problem hiding this comment.
This is a bit annoying, I think now you could also fix it in the parser (use k0 instead of k0_opt)? It looks like we might be able to get rid of k0_opt.
lib/AArch64Base.ml
Outdated
| pp_memo memo ^ " " ^ | ||
| pp_vsimdreg v r1 ^ "," ^ | ||
| pp_vsimdreg v r2 ^ "," ^ | ||
| do_pp_idx true ra idx in |
There was a problem hiding this comment.
This is getting a bit complicated but if you made the change below you wouldn't have to change the hashes of tests with LDP/STP. I think this means that we need to change the declaration of do_pp_idx and make sure that the argument from_pair is set to true for non-neon pair instructions only.
| do_pp_idx true ra idx in | |
| do_pp_idx false ra idx in |
There was a problem hiding this comment.
In fact pp_smemp is used by Neon pair instructions only, so it is safe to change it to do_pp_idx false ra idx or use existing helper pp_idx ra idx. And, yes, once changed no need to update hashes.
murzinv
force-pushed
the
use-idx-for-neon
branch
from
e36f849
to
2dd1e78
Compare
herd/AArch64Sem.ml
Outdated
| get_ea_reg rA v kr sext s ii >>= fun addr -> | ||
| simd_str access_size addr r1 ii |
There was a problem hiding this comment.
This has the unfortunate consequence of changing the intra-instruction relations. So I would suggest instead doing this:
| get_ea_reg rA v kr sext s ii >>= fun addr -> | |
| simd_str access_size addr r1 ii | |
| let ma = get_ea_reg rA v kr sext s ii in | |
| simd_str access_size ma r1 ii |
There was a problem hiding this comment.
And the same applies to all store variants in this PR. The loads are ok.
herd/AArch64Sem.ml
Outdated
| let simd_str_p sz addr rd rs k ii = | ||
| read_reg_neon true rd ii >>= fun v -> |
There was a problem hiding this comment.
Check my comment below but to retain the iico_* relations as before, then you could do:
| let simd_str_p sz addr rd rs k ii = | |
| read_reg_neon true rd ii >>= fun v -> | |
| let simd_str_p sz ma rd rs k ii = | |
| ma >>| | |
| read_reg_neon true rd ii >>= fun (addr, v) -> |
murzinv
force-pushed
the
use-idx-for-neon
branch
from
c2ebd6c
to
4aa36f8
Compare
I have no objection, |
|
@murzinv can you please rebase on top of master and then we can merge this. |
Currently Neon load/store is split in two instructions: one for handling immediate/register offset and another for handling post-index variant. The similar applies to Neon load/store pair. Note, missing support for pre-index variant for Neon load/store. Let's refactor those instructions to take advantage of existing infrastructure for handling addressing modes. That removes need for special instruction for post-index variant and as a bonus naturally brings support for pre-index variant for Neon load/store. Signed-off-by: Vladimir Murzin <[email protected]>
We can improve processing of LDUR instruction by eliminating special handling of the optional immediate. Parser provides `k0` rule which already handle absence of immediate offset by defaulting it to zero, so use it instead of `k0_opt`. Since there are no more users of `k0_opt` left, kill it for good. Signed-off-by: Vladimir Murzin <[email protected]>
murzinv
force-pushed
the
use-idx-for-neon
branch
from
4aa36f8
to
cfbfb70
Compare
|
Thanks @murzinv! |
Currently Neon load/store is split in two instructions: one for handling immediate/register offset and another for handling post-index variant. The similar applies to Neon load/store pair. Note, missing support for pre-index variant for Neon load/store.
Let's refactor those instructions to take advantage of existing infrastructure for handling addressing modes. That removes need for special instruction for post-index variant and as a bonus naturally brings support for pre-index variant for Neon load/store.