Track frame layout changes. (cont.d) #1204
Conversation
|
Yes, I'll be looking at it since it's still fresh in my mind. Thanks for picking this up! |
cranelift-codegen/src/isa/x86/abi.rs
Outdated
| .and_modify(|insts| { | ||
| *insts = insts | ||
| .into_iter() | ||
| .map(|x| *x) |
There was a problem hiding this comment.
Can this be replaced with a .clone()?
| cfa_state.current_depth += word_size; | ||
| cfa_state.cf_ptr_offset -= word_size; | ||
| // And now that we're going to overwrite `rbp`, `rsp` is the only way to get to the call frame. | ||
| cfa_state.cf_ptr_reg = RU::rsp as RegUnit; |
There was a problem hiding this comment.
I feel I've asked this before, but: don't we need to "publish" the changes as FrameLayoutChange here? (and below when popping out nonvolatile regs)
There was a problem hiding this comment.
We do need to record the layout change here, and do just that. It's definitely not immediately obvious since tracks frame layout after pop rbp but isn't applied until popping other registers.
I added some comments to try clarifying this for future readers.
| } | ||
|
|
||
| /// Set of frame layout changes. | ||
| pub type FrameLayoutChanges = Box<[FrameLayoutChange]>; |
There was a problem hiding this comment.
I guess we need data in the first place, but I wonder if using a SmallVec here wouldn't be as efficient and as simple. This looks a bit like premature optimization, since as far as I recall there's no good justification of it being a problem or causing too many allocations. We can let it as is right now, but using the simplest thing first and then revisiting it if it's a problem would be a better strategy.
iximeow
force-pushed
the
frame-layout
branch
from
bd8b316 to
66194d5
Compare
Emphasis mine, on the part I missed when I saw the notification last week :) Another small round of tweaks in line with comments. @bnjbvr if #1204 (comment) and the comments referenced there read well to you I'll squash, merge, and get back to #902! |
Co-Authored-By: bjorn3 <[email protected]>
Co-Authored-By: bjorn3 <[email protected]>
|
@iximeow Tip: If you switch to the "files changed" tab, you can queue multiple suggestions to be committed together in a single commit. |
|
Ah that would have been good, I was wondering about that.. thanks. |
here.
description becomes long, the matter should probably be discussed in an issue
first.
If you don't know who could review this, please indicate so and/or ping
bnjbvr. The list of suggested reviewers on the right can help you.Continuation of #679 as Yury asked I open a new PR from my fork.
This PR teaches
cranelift-codegento track changes to a call frame layout, on top of which we can generate unwind and debug information (as done in #902)@bnjbvr you were looking at this most recently in #679, so I figure you might be most interested in reviewing?