Wasmtime GitHub notifications bot (Sep 03 2025 at 18:24): Wasmtime GitHub notifications bot (Sep 03 2025 at 18:24):pnodet opened PR #11599 from pnodet:pnodet-9 to bytecodealliance:main:
This PR introduces granular call instruction classification in Cranelift's machine instruction layer, enabling more precise function type analysis and potential optimization opportunities.
Changes
- Added
CallTypeenum to distinguish between regular calls, tail calls, and non-calls
- IntroducedFunctionTypeenum for enhanced function classification (Leaf,TailCallOnly,NonLeaf)
- Replaced booleanis_call()with more descriptivecall_type()method in MachInst trait
- Updated all ISA backends (AArch64, x64, RISC-V 64, s390x, Pulley) to implementcall_type()methodAdds core infrastructure for distinguishing between regular calls and tail calls at the instruction level.
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:24):pnodet requested cfallin for a review on PR #11599.
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:24):pnodet requested wasmtime-compiler-reviewers for a review on PR #11599.
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:40):cfallin submitted PR review:
Thanks! Some comments below.
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:40):cfallin created PR review comment:
rustdoc formatting nit: blank line between summary and details (here and below)
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:40):cfallin created PR review comment:
A few naming thoughts:
FunctionTypeevokes something more like a function signature to me; maybeFunctionCallsfor this enum?- Once we call it that, the arms might be a little clearer as
FunctionCalls::None,FunctionCalls::TailOnly, andFunctionCalls::Regular.
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:40):cfallin created PR review comment:
Can we make these enums
pub(crate)? There's no reason to export them to the world (we don't support external backends).
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:40):cfallin created PR review comment:
This is, properly speaking, a property of the ABI -- some may require frame setup for every function, some may not, some may support frame-less tail calls, etc. Can we move the logic to
machinst/abi.rsinstead?
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:40):cfallin created PR review comment:
No need for little tests like this on helper methods that are just
matches on an enum -- they don't add any value but rather they are just repeating the definition.
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:40):cfallin created PR review comment:
This appears to be used only in tests and in passing to
compute_frame_layout, and the latter can just take the enum type directly; could we remove this method?
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:40):cfallin created PR review comment:
Rather than ad-hoc merge logic like this, it would be better to add a method
updateon theFunctionType(orFunctionCalls) that takes aCallType. Then you can havelet mut function_calls = FunctionCalls::None;above the loop, andfunction_calls.update(self.insts[i].call_type());in the loop, and all the logic is placed close to where the enum domains are defined.
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:40):cfallin created PR review comment:
This doesn't seem to be used anywhere -- can we remove it? Best not to have methods that try to summarize/coarsen the enum domain unless we have a precise definition of what we actually want and a place that needs it; let the modeled information domain stand on its own otherwise.
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:40):cfallin created PR review comment:
Rather than pass the bool, let's pass the enum directly. Then the ABI can decide what it wants to do about tails vs regular calls, etc.
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:57):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Sep 03 2025 at 18:57):bjorn3 created PR review comment:
Do we actually need to make a distinction between
LeafandTailCallOnly?
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:02):pnodet updated PR #11599.
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:04):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:04):cfallin created PR review comment:
We don't right now. I was interpreting this PR as a refactor along the way to eventually doing so, and the additional enum arm is minor enough that I don't mind the "YAGNI violation"; but @pnodet can you say more about your plans here?
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:05):cfallin edited PR review comment.
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:08):pnodet submitted PR review.
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:08):pnodet created PR review comment:
I agree for
FunctionType/FunctionCallsenum.But for
CallType, since I added to thepub trait MachInstacall_type()method returningCallTypeI need to either keep it public or add#[allow(private_interface)]to the method
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:11):pnodet updated PR #11599.
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:11):pnodet created PR review comment:
I don't mind FunctionCalls
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:11):pnodet submitted PR review.
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:12):cfallin created PR review comment:
Ah, that's fair -- we aren't consistent currently with our usage of
pub; someone should do a broader pass on that (probably not a good task for a newcomer since it has more to do with our published interface and policy questions) but for nowpubis fine there.
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:12):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:22):pnodet submitted PR review.
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:22):pnodet created PR review comment:
Yes, this changes are not trying to fix anything per se but after working on the is_leaf detection PR I thought it could help for improvements and optimizations in the future. I don't think it introduces much and open to discussion as I didn't really go through an issue -> pr process
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:26):pnodet submitted PR review.
Wasmtime GitHub notifications bot (Sep 03 2025 at 19:26):pnodet created PR review comment:
They are no optimizations made here so this PR could be regarded as a in between step / preparation phase
Wasmtime GitHub notifications bot (Sep 03 2025 at 22:25):pnodet updated PR #11599.
Wasmtime GitHub notifications bot (Sep 03 2025 at 22:26):pnodet submitted PR review.
Wasmtime GitHub notifications bot (Sep 03 2025 at 22:26):pnodet created PR review comment:
I gave this a bit of thought and made a pass in this commit https://github.com/bytecodealliance/wasmtime/pull/11599/commits/bbbbf0ec0a9761eaff1d5d00440c155e17001afc
Wasmtime GitHub notifications bot (Sep 03 2025 at 22:36):bjorn3 created PR review comment:
Once the optimization is done, we can change back to an is_leaf bool where true could be either no calls or only tail calls, right? Doing the is_call change and optimization in one step would avoid most of the changes in this PR, right?
Wasmtime GitHub notifications bot (Sep 03 2025 at 22:36):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Sep 03 2025 at 22:44):cfallin created PR review comment:
@bjorn3 that's not totally clear to me at least -- for example, if the tail-calls result in an outgoing argument area of nonzero size, that precludes skipping the frame, whereas a tail call with args in registers only can still avoid the frame (if no other clobber-saves or stackslots). So the ABI logic may need to handle it specially.
Wasmtime GitHub notifications bot (Sep 03 2025 at 22:44):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 03 2025 at 22:47):cfallin submitted PR review:
Thanks!
Wasmtime GitHub notifications bot (Sep 03 2025 at 22:47):cfallin has marked PR #11599 as ready for review.
Wasmtime GitHub notifications bot (Sep 03 2025 at 23:08):cfallin merged PR #11599.
Last updated: Dec 06 2025 at 06:05 UTC