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

Add Zaamo and Zalrsc #384

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Add Zaamo and Zalrsc #384

merged 1 commit into from
Jun 14, 2024

Conversation

ved-rivos
Copy link
Contributor

@ved-rivos ved-rivos changed the title Add unratified Zaamo and Zalrsc Add Zaamo and Zalrsc Apr 27, 2024
@rpsene
Copy link
Contributor

rpsene commented Jun 11, 2024

@billmcspadden-riscv this spec was also recently ratified. Can this be merged?

@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 11, 2024

What is the official stance on A vs Za*? LLVM doesn’t believe that A implies Zalrsc or Zaamo, yet this PR says it does.

@ved-rivos
Copy link
Contributor Author

ved-rivos commented Jun 11, 2024

The A extension comprises instructions provided by the Zaamo and Zalrsc extensions.

The LLVM PR for this is open: llvm/llvm-project#77424 and this one is merged: llvm/llvm-project#78970

@Alasdair
Copy link
Collaborator

I think we merged a PR recently that moved the guards to be more in line with other files, so I think this PR needs to be rebased for that.

Other than that I don't see any issues here. Presumably at some point we will have to change the haveZalrsc and haveZaamo implementations to allow toggling them individually.

@ved-rivos
Copy link
Contributor Author

Ideally PRs like the one the one to refactor the *aext.sail would have been done after applying the related PRs like a) for Zabha, b) Zacas, c) Zaamo, d) Zalrsc that have been queued for a while now. If these had been applied before the refactoring then the refactoring could have more comprehensively addressed these ratified extensions. I will try to get these four reworked/rebased and retested soon.

@Timmmm
Copy link
Collaborator

Timmmm commented Jun 12, 2024

Open PRs aren't actually a queue though. This one looks very easy to rebase in any case.

@ved-rivos
Copy link
Contributor Author

Open PRs aren't actually a queue though.

Open PRs are not a queue but rewriting upstream for aesthetic reasons when there are queued PRs is not kind.

Copy link

Test Results

712 tests  ±0   712 ✅ ±0   0s ⏱️ ±0s
  6 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit b7dc749. ± Comparison against base commit ba35af5.

@Timmmm
Copy link
Collaborator

Timmmm commented Jun 13, 2024

Open PRs are not a queue but rewriting upstream for aesthetic reasons when there are queued PRs is not kind.

It wasn't entirely aesthetic, and I don't think we should pause refactoring if any open PR conflicts, but in any case it wasn't my intention to be unkind or cause unnecessary rebasing (which is a right pain indeed!). Sorry about that.

I also wasn't aware of this PR tbf - would be nice if Github could highlight conflicting PRs automatically!

Anyway this LGTM and seems to be entirely uncontroversial so I'll merge it tomorrow if nobody has any objections...

@Timmmm Timmmm merged commit 0e9850f into riscv:master Jun 14, 2024
2 checks passed
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.

6 participants