-
Notifications
You must be signed in to change notification settings - Fork 61
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
[all]: Rework Neon load/store and load/store pair #804
Conversation
Following #749 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments but otherwise looks good to me.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
e36f849
to
2dd1e78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two quick comments
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the same applies to all store variants in this PR. The loads are ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
c2ebd6c
to
4aa36f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thanks for the quick fix!
If @maranget doesn't see any problems with the hash change we can merge. Let's give people some time to react. Let's aim to have this merged by the end of Thursday?
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]>
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.