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 Cortex M4 #242

Merged
merged 1 commit into from
Oct 3, 2022
Merged

ARM Cortex M4 #242

merged 1 commit into from
Oct 3, 2022

Conversation

sarranz
Copy link
Collaborator

@sarranz sarranz commented Sep 9, 2022

No description provided.

Copy link
Contributor

@bgregoir bgregoir left a comment

Choose a reason for hiding this comment

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

I am very impress. Sorry for all the small comments.

x = #BIC(x, -1);

// Shifts.
x = #BIC(x, arg0, #LSL(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I do not like the syntax #LSL(0).
I think it is better to use arg0 << 0

Copy link
Contributor

Choose a reason for hiding this comment

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

I will not block the PR for this. But at least we should discuss it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we will need some lowering for Copn arguments (should be easy with the existing function in arm_lowering, I did not complete it to get these changes merged as soon as possible).

compiler/src/glob_options.ml Show resolved Hide resolved
compiler/src/help.ml Outdated Show resolved Hide resolved
let { L.pl_loc = loc ; L.pl_desc = s } = id in
let name, sz = extract_size s in
(* TODO_ARM: Merge this. *)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the impression that the change in the code
comes from #LSL(0) argument is it true ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Partly. To emit an ARM instruction we need to look for the mnemonic (e.g. ADD) but also for the S and cc suffixes (specifying whether it set flags or nor) and then the arguments (to check if they have a shifter register as an argument). That's why the tt_prim function now takes the arguments as well: ADD x, y, z << 2 needs to become ADD x, y, z, 2 and be tagged as having a LSL shift. Maybe there is a better way of doing this.

@@ -1369,7 +1377,9 @@ let tt_prim asmOp ws id =
| PrimV pr -> (match sz with SAv (s, ve, sz) -> pr ws s ve sz | _ -> rs_tyerror ~loc (PrimIsVector s))
| PrimX pr -> (match sz with SAx(sz1, sz2) -> pr ws sz1 sz2 | _ -> rs_tyerror ~loc (PrimIsX s))
| PrimVV pr -> (match sz with SAvv (ve, sz, ve', sz') -> pr ws ve sz ve' sz' | _ -> rs_tyerror ~loc (PrimIsVectorVector s))
| PrimARM _ -> failwith "tt_prim ARM M4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it expected ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ARM case should be handled by the target_arch match. After the merge I will write a single tt_prim function that deals with all architectures uniformly.

proofs/arch/asm_gen_proof.v Show resolved Hide resolved
proofs/lang/utils.v Outdated Show resolved Hide resolved
proofs/lang/utils.v Outdated Show resolved Hide resolved
@@ -6,10 +6,65 @@ Set Implicit Arguments.
Unset Strict Implicit.
Unset Printing Implicit Defensive.

Lemma disjoint_subset_diff xs ys :
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, my remark is not related to this commit.
Why those proofs are not done for all set, i.e it is done only for Sv.

Copy link
Collaborator Author

@sarranz sarranz Sep 13, 2022

Choose a reason for hiding this comment

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

Do you mean this should be a module like MSetEqProperties.EqProperties and then define something like SvP? Sorry, I'm not used to Coq modules yet

proofs/compiler/compiler_proof.v Outdated Show resolved Hide resolved
@vbgl
Copy link
Member

vbgl commented Sep 28, 2022

I’m going to merge this pull request soon. Can I squash all the commits in this branch into a single commit?

@sarranz
Copy link
Collaborator Author

sarranz commented Sep 28, 2022

I’m going to merge this pull request soon. Can I squash all the commits in this branch into a single commit?

I have no objection to squashing. But I still haven't pushed the changes to the parser we discussed because I don't know if the approach I mentioned on Zulip is viable. However I could also do it after merging.

@eponier
Copy link
Contributor

eponier commented Sep 28, 2022

I have no objection to squashing. But I still haven't pushed the changes to the parser we discussed because I don't know if the approach I mentioned on Zulip is viable. However I could also do it after merging.

The idea is to make a release just before merging your branch, so that we have some time to fix and polish things, so I'd suggest to merge that as soon as possible and deal with all fixes/improvements in other PRs.

@sarranz
Copy link
Collaborator Author

sarranz commented Sep 29, 2022

I have no objection to squashing. But I still haven't pushed the changes to the parser we discussed because I don't know if the approach I mentioned on Zulip is viable. However I could also do it after merging.

The idea is to make a release just before merging your branch, so that we have some time to fix and polish things, so I'd suggest to merge that as soon as possible and deal with all fixes/improvements in other PRs.

Then I'll do the fixes in separate PRs later.

@vbgl vbgl force-pushed the arm_m4 branch 2 times, most recently from 32c4526 to 1dbdfb5 Compare October 3, 2022 13:39
@vbgl
Copy link
Member

vbgl commented Oct 3, 2022

I’ve excluded the examples/arm_m4 directory from a few tests to let CI go through. This shall be addressed later.

If you like type-class puzzles, here is one: https://gitlab.com/jasmin-lang/jasmin/-/jobs/3119039375 Any volunteer to give a look?

@vbgl vbgl merged commit e2467f3 into main Oct 3, 2022
@vbgl vbgl deleted the arm_m4 branch October 3, 2022 14:32
@vbgl
Copy link
Member

vbgl commented Oct 4, 2022

The typeclass issue arises systematically with Coq 8.16 (and mathcomp 1.15).

@vbgl
Copy link
Member

vbgl commented Oct 4, 2022

See #261 for a quick-and-dirty fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants