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.

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

make jalv code consistent with the comment #65

Closed
wants to merge 1 commit into from

Conversation

morganthomas
Copy link
Collaborator

@morganthomas morganthomas commented Oct 12, 2023

jalv in the design doc is described as adding c (the value of the third operand) to fp. In the comment on the execution code for jalv, it says jalv sets fp to [c] (the value of the third operand). In the code, it sets fp to fp+[c]. The LLVM Valida backend, on the other hand, assumes that jalv sets fp to [c]. This PR changes jalv to set fp to [c].

@morganthomas morganthomas marked this pull request as ready for review October 12, 2023 02:59
@maxgillett
Copy link
Member

Hi Morgan, thanks for the PR. The page you linked is referencing an outdated spec. The official one is here: valida-xyz/valida-compiler#2, which is consistent with the code. I also believe the compiler issue you referenced was already resolved.

Is there a reason why we should consider moving to an absolute instead of a relative frame pointer reference for this instruction? We don't currently have a way to read the current frame pointer value, so going this route would require more changes to be made.

@morganthomas
Copy link
Collaborator Author

Hi @maxgillett . The reason I'm suggesting this change is that the LLVM compiler assumes that jalv works as it would in this PR. Also, the spec you've linked is not obviously-to-me consistent with itself on this point. In the calling convention section, the spec says that return FP is stored on the stack, at fp+8. My thought then would be to load the value at fp+8 into fp as part of transferring control to the caller, say using jalv.

I'm doing this work as part of resolving lita-xyz/llvm-valida/issue/4, which is about getting a basic example of LLVM compiler output to run and work as expected. This change seems to help, although the issue isn't entirely resolved yet, I think. Certainly I think that the instructions the compiler emits to return from a function do not make sense without this change to the semantics of jalv. I think that Michael had the proposed semantic for jalv in mind when he wrote the compiler.

We can look into what it would take to modify the compiler to account for the current semantics of jalv. I'm just not entirely sure that the spec as written is implementable. It seems to me to call for loading a stored value of fp from the stack, and if there's no instruction that lets us do that, then we can't implement the Valida calling convention on Valida.

@maxgillett
Copy link
Member

I see, the calling convention section there should be updated to read something like "return FP offset is stored at fp+8". I'll make that change. I'm still confused though about why an absolute return is preferable over a relative offset.

I don't have access to that Lita repo so can't see the details of the issue you're referencing, but I would assume that this is a bug in Michael's code. We had previously corresponded over email about the semantics of jal/jalv, and seemed to be on the same page, but I haven't yet looked at his code in detail. I'm also not sure what you mean when you say the spec is not implementable. The jalv instruction doesn't load a stored value of fp, but an offset (which is always stored at 8(fp)), and which is set before calling using an imm instruction. Maybe that's the source of confusion? The compiler I've written also does handle the current calling convention, albeit it's a bit hacky, as I'm not a compiler engineer.

@morganthomas
Copy link
Collaborator Author

Okay, I understand. I'm not saying that the proposed semantics of jalv is preferable. With the correction you're going to make to the spec, I think the spec will be implementable. It would put the caller in the position of needing to know the length of their stack frame at compile time, which is not always possible. I understand that what we've said about this is that we should make it possible to know the stack frame size by letting alloca be malloc, which should result in stack frame size being knowable at compile time.

I didn't realize that you wrote a compiler implementation separate from Michael's. I assume that is this. I'll take a look at how you've done it.

@morganthomas morganthomas mentioned this pull request Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants