Teach rustc about the Xtensa VaListImpl #127565
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
r? @davidtwco |
|
cc @workingjubilee who has been doing a lot of varargs work recently |
There was a problem hiding this comment.
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.
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
Gentle ping for a review whenever you have time some spare cycles :) |
There was a problem hiding this comment.
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.
|
@MabezDev |
MabezDev
force-pushed
the
xtensa-vaargs
branch
from
bcc9c0e
to
c711221
Compare
|
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
added
S-waiting-on-review
|
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. |
MabezDev
force-pushed
the
xtensa-vaargs
branch
from
c711221
to
710b544
Compare
rustbot
removed
the
S-waiting-on-review
rustbot
added
the
S-waiting-on-author
…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
force-pushed
the
xtensa-vaargs
branch
from
710b544
to
ec0e3dd
Compare
|
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. |
| // The first time we switch from regsave to stack we needs to adjust our offsets a bit. | ||
| // va_stk is set up such that the first stack argument is always at va_stk 32. | ||
| // The corrected offset is written back into the va_list struct. | ||
|
|
||
| // let offset_corrected = cmp::max(offset, 32); | ||
| let needs_correction = bx.icmp(IntPredicate::IntULE, offset, regsave_size); | ||
| let offset_corrected = bx.select(needs_correction, bx.const_i32(32), offset); |
There was a problem hiding this comment.
If we actually want the max semantics, then this needs to also compare against bx.const_i32(32) and not regsave_size.
| // let offset_next_corrected = if offset <= 24 { | ||
| // 32 slot_size | ||
| // } else { | ||
| // offset slot_size | ||
| // }; | ||
| // va_ndx = offset_next_corrected; | ||
| let offset_next_corrected = | ||
| bx.select(needs_correction, bx.const_i32(32 slot_size), offset_next); |
There was a problem hiding this comment.
The reason I pointed out this framing is that this makes it more obvious that this expression should be equivalent to just
let offset_next_corrected = offset_corrected slot_size;and thus should be
| // let offset_next_corrected = if offset <= 24 { | |
| // 32 slot_size | |
| // } else { | |
| // offset slot_size | |
| // }; | |
| // va_ndx = offset_next_corrected; | |
| let offset_next_corrected = | |
| bx.select(needs_correction, bx.const_i32(32 slot_size), offset_next); | |
| // let offset_next_corrected = offset_corrected slot_size; | |
| // va_ndx = offset_next_corrected; | |
| let offset_next_corrected = bx.add(offset_next, slot_size); |
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.