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.

Sign up for GitHub

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

Jump to bottom

Add asm!() support for PowerPC #84732

Merged
merged 1 commit into from
May 13, 2021
Merged

Add asm!() support for PowerPC #84732

merged 1 commit into from
May 13, 2021

Conversation

DrChat
Copy link
Contributor

@DrChat DrChat commented Apr 30, 2021 •
edited
Loading

This includes GPRs and FPRs only.
Note that this does not include PowerPC64.

For my reference, this was mostly duplicated from PR #73214.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 30, 2021
@petrochenkov
Copy link
Contributor

r? @Amanieu

Amanieu
Amanieu reviewed May 2, 2021
View reviewed changes
Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

The main issue is that you need to disallow the use of LLVM reserved registers. Refer to LLVM's getReservedRegisters for the full list.

@DrChat
Copy link
Contributor Author

DrChat commented May 2, 2021

Thanks for the review! I'm still very unfamiliar with the compiler, so I copied another similar pull request almost verbatim.

I'm a bit confused about reserved registers - does marking them as reserved simply prevent users from using them as in/out parameters to the asm!() macro?

@Amanieu
Copy link
Member

Amanieu commented May 2, 2021

Yes, we mark them with #error in the macro so to prevent the user from using them as inputs/outputs. This is necessary because LLVM will silently generate invalid code if a reserved register is used as an inline asm operand.

@DrChat
Copy link
Contributor Author

DrChat commented May 2, 2021

Should I add any tests to verify the compiler outputs errors for the reserved registers?

Amanieu
Amanieu reviewed May 3, 2021
View reviewed changes
Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

In the docs, add entries in the "Template modifiers" section: this should indicate that PowerPC registers are rendered as plain integers in inline assembly without any prefix letters.

Amanieu
Amanieu reviewed May 3, 2021
View reviewed changes
@DrChat DrChat requested a review from Amanieu May 3, 2021 23:54
@DrChat DrChat mentioned this pull request May 4, 2021
Merged
@DrChat DrChat requested a review from Amanieu May 7, 2021 15:35
@DrChat
Copy link
Contributor Author

DrChat commented May 7, 2021

Looks like we got the fix merged for the LLVM bug :)

@Amanieu
Copy link
Member

Amanieu commented May 7, 2021

You need to update the llvm-project submodule in this PR to actually pick up the fix.

@DrChat
Copy link
Contributor Author

DrChat commented May 7, 2021

Ah - dang. I didn't catch that it was a submodule. Will do in a few jiffies.

@DrChat
Copy link
Contributor Author

DrChat commented May 7, 2021

Alright - I verified locally that my test is fixed.

@Amanieu
Copy link
Member

Amanieu commented May 8, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 8, 2021

📌 Commit 3a2df79b55f16a6ac0d18b26a2de58beacb64458 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2021
@DrChat DrChat requested a review from Amanieu May 9, 2021 01:28
@Amanieu
Copy link
Member

Amanieu commented May 9, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 9, 2021

📌 Commit c03e125fc71bcf40db8547f34c65eab56e839bf4 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2021
@bors
Copy link
Contributor

bors commented May 9, 2021

⌛ Testing commit c03e125fc71bcf40db8547f34c65eab56e839bf4 with merge 293f82850c006376f8bdc124b2cb371c20a51283...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@DrChat
Copy link
Contributor Author

DrChat commented May 9, 2021 •
edited
Loading

Ack. I didn't find and replace the ones with [1-31]. Pushing a fix now.

@bors
Copy link
Contributor

bors commented May 9, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 9, 2021
@bors
Copy link
Contributor

bors commented May 9, 2021

☔ The latest upstream changes (presumably #83894) made this pull request unmergeable. Please resolve the merge conflicts.

@DrChat
Add initial asm!() support for PowerPC
b1bb5d6 
This includes GPRs and FPRs only
@DrChat
Copy link
Contributor Author

DrChat commented May 12, 2021

Rebased the branch and it should be good to go.

@Amanieu
Copy link
Member

Amanieu commented May 12, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 12, 2021

📌 Commit b1bb5d6 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 12, 2021
@bors
Copy link
Contributor

bors commented May 13, 2021

⌛ Testing commit b1bb5d6 with merge 72d0725...

@bors
Copy link
Contributor

bors commented May 13, 2021

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 72d0725 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 13, 2021
@bors bors merged commit 72d0725 into rust-lang:master May 13, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 13, 2021
@DrChat DrChat mentioned this pull request May 14, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 16, 2021
@bors
Auto merge of rust-lang#85279 - DrChat:asm_powerpc64, r=Amanieu
e78bccf 
Add asm!() support for PowerPC64

I was anticipating this to be difficult so I didn't do it as part of rust-lang#84732... but this was pretty easy to do 👀
@KerryRJ KerryRJ mentioned this pull request Aug 9, 2021
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Amanieu Amanieu Awaiting requested review from Amanieu

Assignees

@Amanieu Amanieu

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.
7 participants
@DrChat @rust-highfive @petrochenkov @Amanieu @bors @rust-log-analyzer @rustbot