Repository navigation
Add support for dynamic memory allocation on stack (localloc support) - #191
Open
Krishna-13-cyber wants to merge 4 commits into
Open
Krishna-13-cyber wants to merge 4 commits into
Krishna-13-cyber wants to merge 4 commits into
Conversation
saitama951
reviewed
Oct 28, 2026
|
|
||
| if (amount == 0) | ||
| { | ||
| emit->emitIns_R_I(INS_lgfi, EA_8BYTE, targetReg, 0); |
Collaborator
There was a problem hiding this comment.
if the amount is 0, why do we even load something?
where is this targetReg is referenced is what I'm trying to understand?
Author
There was a problem hiding this comment.
Initially, I had the same thing in mind to bypass the 0 size condition but if this block is being call multiple times, there would redundant backchaining happening for zero size. (Just to remain in as sync with other archs).
For the load part, we do need so that we are sure there is no previous value we are taking in, targetReg is whatever register LSRA allocates to us, that might have previous values. The node must always leave a defined value in targetReg.
Without the lgfi targetReg, 0, the register would hold prev stale values in it, and the parent node would read that stale value as the stackalloc result. Also once we know its size 0, we return early and dont want to do the long way of backchaining.
If we scrap this altogether, we dont need lgfi at all, we can do this but I preferred to keep the zeroing case in the constant logic.
Collaborator
|
what does targetReg exactly do here? we preserve the backchain pointer, and then we says targetReg = SP+ 160 + outgoing arg space. |
Author
For this, I am not sure how we can use SP_BASED or FP_BASED here, as we know the SP_Based is much volatile and changes for dynamic memory. I think SP_Based can be ruled out. We have to request a register which lsra allocated as using the same as FP_BASED would not suffice as we are pointing to an address with space over and above the RSA+lva space. FP is already pointing to the caller's frame. So if you mean offsetting from that address, I will have to check that. targetReg is being used to point to the newly allocated memory buffer, right above the RSA+lva which is the framebelow. |
Collaborator
|
The allocated dynamic memory area should be pointed by the FP (what I mean targetReg should always be the FP) |
Author
According to the ABI, it needs to be placed above the RSA region and below the caller's frame pointer (FP). Using FP to point to targetReg would not be the correct convention, as FP needs to maintain its role in referencing the previous frame. Moving FP to point to targetReg or another location within the dynamic memory slot would shift the relative offsets of arguments and local variables with respect to FP, potentially breaking the existing frame layout. Additionally, to preserve the correctness of the target buffer, targetReg needs to be maintained in a dedicated register rather than relying on the existing FP- or SP-based relative addressing. Address (High) |
Collaborator
|
Extra comments are addressed offline. |
saitama951
reviewed
Nov 4, 2026
| emit->emitIns_R_R(INS_sgr, EA_8BYTE, REG_SPBASE, alignedReg); | ||
| emit->emitIns_R_R_I(INS_stg, EA_8BYTE, tmpReg, REG_SPBASE, 0); | ||
| emit->emitIns_R_R_I(INS_lay, EA_PTRSIZE, targetReg, REG_SPBASE, frameBelow); | ||
| } |
Collaborator
There was a problem hiding this comment.
we also need to move down the register stack allocated contents below, i.e parameter passing etc.
Author
Running newly added tests for memalloc. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
genLclHeap takes care of dynamic, runtime allocation of memory directly onto the current thread's stack frame.