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

Teach rustc about the Xtensa VaListImpl #127565

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Teach rustc about the Xtensa VaListImpl #127565

wants to merge 1 commit into from

Conversation

MabezDev
Copy link
Contributor

Following on from the target Xtensa target PRs (#125141, #126380), this PR teaches rustc about the structure of the VA list on the Xtensa arch, as well as adding the required lowering to be able to actually use it.

@rustbot
Copy link
Collaborator

rustbot commented Jul 10, 2024

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 10, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor

oli-obk commented Jul 10, 2024

r? @davidtwco

@rustbot rustbot assigned davidtwco and unassigned oli-obk Jul 10, 2024
@tgross35
Copy link
Contributor

cc @workingjubilee who has been doing a lot of varargs work recently

workingjubilee
workingjubilee requested changes Jul 11, 2024
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the codegen changes in detail as things will go much more smoothly if I have the bits of information I am asking for details on. If you don't have all this information, that's fine, and don't go off for a year digging for answers or anything. I just want as much of it that is on-hand and easily referenced.

library/core/src/ffi/va_list.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Show resolved Hide resolved
@tgross35
Copy link
Contributor

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 16, 2024
@MabezDev
Copy link
Contributor Author

MabezDev commented Sep 3, 2024

Gentle ping for a review whenever you have time some spare cycles :)

@workingjubilee workingjubilee self-assigned this Sep 3, 2024
@davidtwco davidtwco removed their assignment Sep 6, 2024
workingjubilee
workingjubilee requested changes Sep 9, 2024
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

At each step, I prefer to confirm the code implements the spec rather than read the code and try to discern what spec it implements. There's only so many ways to implement a spec, but there's an unbounded set of specs a given fragment could implement. So, this could use a few more comments.

Perhaps a less overloaded naming scheme would suffice? But when we're already checking in about 100 lines of code, we can afford a bit of extra padding to smooth things for the next reader.

compiler/rustc_codegen_llvm/src/va_arg.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Outdated Show resolved Hide resolved
library/core/src/ffi/va_list.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Show resolved Hide resolved
@alex-semenyuk
Copy link
Member

@MabezDev
From wg-triage. Do you have any updates on this PR?

@MabezDev
Copy link
Contributor Author

Thank you for the review @workingjubilee (and sorry for the delay), I think I've addressed most of your initial concerns. I'm a little unsure on how to improve the stack ndx check, so any ideas would be appreciated.

@alex-semenyuk alex-semenyuk added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 28, 2024
@workingjubilee
Copy link
Member

Don't worry too much if we don't figure out anything to change. I expect nonspecific responses... including no response... to review comments that don't say anything particularly specific! In this case, it was mostly to signal what I am going to give further attention later when the code was structured to make it easier to reach that point, without feeling as-distracted by the smaller details of the code preceding.

In a sense it was an invitation for you to have a sudden flash of insight if you had one to spare... it's completely understandable if you spent that on something else, like the meaning of love, attaining nirvana, or the best wine-and-cheese pairing for an appetizer. Otherwise it was mostly a reminder to myself.

workingjubilee
workingjubilee requested changes Nov 4, 2024
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Currently I still feel like I'm missing something or misreading something, and there is at least one clear omission.

compiler/rustc_codegen_llvm/src/va_arg.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/va_arg.rs Outdated Show resolved Hide resolved
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 4, 2024
@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 4, 2024
@MabezDev
Copy link
Contributor Author

MabezDev commented Nov 6, 2024

Thank you for the review! You were correct in your understanding of the LLVM codegen API, and I have added the relevant comments above the blocks in Rust code. This was a nice suggestion as it makes it a lot more readable.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@MabezDev
Copy link
Contributor Author

Thanks for bearing with me, and sorry for the miscommunication. I believe I've addressed your comments properly now, let me know if there is anything else you'd like me to adjust.

workingjubilee
workingjubilee approved these changes Dec 2, 2024
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

No problem!

I believe this is correct.

compiler/rustc_codegen_llvm/src/va_arg.rs Show resolved Hide resolved
@workingjubilee
Copy link
Member

r=me with the typo settled. :3

@bors delegate+

@kelnos @jothan
@plietar @MabezDev
Teach rust core about Xtensa VaListImpl and add a custom lowering of …
059f627 
…vaarg for xtensa.

LLVM does not include an implementation of the va_arg instruction for
Xtensa. From what I understand, this is a conscious decision and
instead language frontends are encouraged to implement it themselves.
The rationale seems to be that loading values correctly requires
language and ABI-specific knowledge that LLVM lacks.

This is true of most architectures, and rustc already provides
implementation for a number of them. This commit extends the support to
include Xtensa.

See https://lists.llvm.org/pipermail/llvm-dev/2017-August/116337.html
for some discussion on the topic.

Unfortunately there does not seem to be a reference document for the
semantics of the va_list and va_arg on Xtensa. The most reliable source
is the GCC implementation, which this commit tries to follow. Clang also
provides its own compatible implementation.

This was tested for all the types that rustc allows in variadics.

Co-authored-by: Brian Tarricone <[email protected]>
Co-authored-by: Jonathan Bastien-Filiatrault <[email protected]>
Co-authored-by: Paul Lietar <[email protected]>
@MabezDev
Copy link
Contributor Author

MabezDev commented Dec 3, 2024

@bors r=workingjubilee

@MabezDev
Copy link
Contributor Author

MabezDev commented Dec 4, 2024

I think I broke bors, let me try again 😅.

@bors r=workingjubilee

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee approved these changes

Assignees

@workingjubilee workingjubilee

Labels
O-xtensa S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@MabezDev @rustbot @rust-log-analyzer @oli-obk @tgross35 @alex-semenyuk @workingjubilee @davidtwco @kelnos