Skip to content

add support for riscv64#390

Closed
futurejones wants to merge 1 commit intoapple:mainfrom
futurejones:add-riscv64-support
Closed

add support for riscv64#390
futurejones wants to merge 1 commit intoapple:mainfrom
futurejones:add-riscv64-support

Conversation

@futurejones
Copy link
Copy Markdown
Contributor

Add support for riscv64 as host architecture.

Checklist

  • I've read the Contribution Guidelines
  • My contributions are licensed under the Swift license.
  • I've followed the coding style of the rest of the project.
  • I've added tests covering all new code paths my change adds to the project (if appropriate).
  • I've added benchmarks covering new functionality (if appropriate).
  • I've verified that my change does not break any existing tests or introduce unexplained benchmark regressions.
  • I've updated the documentation if necessary.

@futurejones futurejones requested a review from lorentey as a code owner June 17, 2024 05:32
@futurejones
Copy link
Copy Markdown
Contributor Author

@lorentey any progress?

Copy link
Copy Markdown
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

Sure!

The caveat is that we cannot guarantee that the code will continue to work as is on this architecture -- we don't have CI set up for RISC-V, nor capacity to manually run tests on it.

@lorentey
Copy link
Copy Markdown
Member

lorentey commented Jul 8, 2024

Did you intentionally target this PR on the main branch, rather than on the release/1.1 series?

@futurejones
Copy link
Copy Markdown
Contributor Author

@lorentey in the other parts of the swift project I have always submitted PR's to the main branch and then cherrypicked to release/ branches as needed. Is your workflow different for this project?
Thanks.

@lorentey
Copy link
Copy Markdown
Member

lorentey commented Aug 1, 2024

Yes. Changes in this repository need to land in the earliest release branch that wants to ship them.

The project maintainer then merges (not cherry-picks, merges) them to downstream branches as needed.

This is explained in the README.

@futurejones
Copy link
Copy Markdown
Contributor Author

@lorentey, apologies, I should have read the README more closely.
Yes this should be targeting the release/1.1 branch.
Can you make this change.
Thanks.

@lorentey
Copy link
Copy Markdown
Member

@futurejones Do you no longer need this change, or were you just unable to retarget it on the release/1.1 branch?

@futurejones
Copy link
Copy Markdown
Contributor Author

@lorentey I don't have write access to this repository so I can't edit and retarget the PR target branch.
That is why I asked you to make the change

Yes this should be targeting the release/1.1 branch.
Can you make this change.
Thanks.

After waiting for nearly 2 months with no response I decided to close the PR rather get stuck in a frustrating cycle of pinging and getting no response.

This is what I have been experiencing with other PR's that I have been trying to get approved in several other swift sub projects.

Whether or not this PR gets approved and merged is not for my need or benefit, it was to improve the swift project support for everyone.

anyway, thanks for responding.

@lorentey
Copy link
Copy Markdown
Member

lorentey commented Sep 24, 2024

@futurejones I see. I can rebase your commit on top of release/1.1 for you if you want. I have no way to verify that the result will work though, and I strongly suspect the CMake change is not going to be enough.

The swift-collections codebase includes a handful of cases where Swift code is conditional on the target architecture, and none of those know about riscv64 as a possibility. To properly support 64-bit RISC-V, I expect these will need to be updated, preferably by refactoring them to use the new _pointerBitWidth condition (we generally have these because we needed to know the size of pointers).

@lorentey lorentey mentioned this pull request Sep 24, 2024
7 tasks
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.

2 participants