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

Fix ccall return value boxing on ARM/AArch64 #23739

Merged
merged 1 commit into from
Sep 20, 2017
Merged

Conversation

yuyichao
Copy link
Contributor

@yuyichao yuyichao commented Sep 17, 2017

We previously rely on the extra allocation from the GC to keep the stores inbounds.
This is broken by the allocation optimization since the stack allocation will only have
the requested bytes and not more.

@yuyichao yuyichao added the compiler:codegen Generation of LLVM IR and native code label Sep 17, 2017
@vtjnash
Copy link
Member

vtjnash commented Sep 17, 2017

Seems like this should be using our llvm_type_rewrite so it doesn't inhibit llvm optimizations.

@yuyichao
Copy link
Contributor Author

We are only storing the value so as long as the size is smaller or the same we don't care what type it actually is so unconditionally going through llvm_type_rewrite is unnecessary and can generate extra code.

As long as we are doing this only when there's a size mismatch (i.e. what this PR is doing), it is exactly the same as llvm_type_rewrite. I didn't use it since most of the useful logic from the function is already here to check the size....

@yuyichao
Copy link
Contributor Author

Actually I missed the trunc case in llvm_type_rewrite so I guess I can do that. It doesn't actually matter for the real code though since the type we have here always have an aggregate....

@vtjnash
Copy link
Member

vtjnash commented Sep 18, 2017

Ah, didn't realize this was just for structs. (In which case, I think llvm_type_rewrite should be doing the same as this code?) Anyways, this is probably fine, was just considering opportunities for consolidating code.

@yuyichao
Copy link
Contributor Author

FWIW, I think this can share code better with memcpy (the only use of the value is storing anyway). I didn't do it since I don't want to have conflict with #23352 but maybe I should just rebase on it.....

@yuyichao yuyichao force-pushed the yyc/codegen/aa64-ccall branch from 658b4de to 9986bfa Compare September 19, 2017 16:00
We previously relies on the extra allocation from the GC to keep the stores inbounds.
This is broken by the allocation optimization since the stack allocation will only have
the requested bytes and not more.
@yuyichao yuyichao force-pushed the yyc/codegen/aa64-ccall branch from 9986bfa to ca28b5f Compare September 19, 2017 20:12
@yuyichao yuyichao merged commit 83a89a1 into master Sep 20, 2017
@yuyichao yuyichao deleted the yyc/codegen/aa64-ccall branch September 20, 2017 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants