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

Implement packing local variable and constant data on 64-bit if stack usage is high #156

Merged
merged 3 commits into from
Sep 6, 2023

Conversation

ksh8281
Copy link
Collaborator

@ksh8281 ksh8281 commented Sep 4, 2023

No description provided.

@ksh8281 ksh8281 requested a review from clover2123 as a code owner September 4, 2023 02:55
@@ -347,7 +347,7 @@ if (f.type == Type::B) { puts("failed in msvc."); }
std::unique_ptr<uint8_t[]> Result##HolderWhenUsingMalloc; \
size_t bytes##Result = (Bytes); \
Type* Result; \
if (LIKELY(bytes##Result < 512)) { \
if (LIKELY(bytes##Result < 2048)) { \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any special reason for increasing the size of alloca?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code generated by emcc sometimes uses stack a lot.

// put already aligned variables first
for (size_t i = m_currentFunctionType->param().size(); i < m_localInfo.size(); i++) {
auto& info = m_localInfo[i];
if (Walrus::hasCPUWordAlingedSize(info.m_valueType) || needsCPUWordAlingedAddress(info.m_valueType)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a typo Alinged -> Aligned

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops!

Comment on lines +449 to +451
case Value::FuncRef:
case Value::ExternRef:
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

little bit confused, do these FuncRef and ExternRef need alignment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arm devices cannot support unaligned pointer well :(
https://embeddedartistry.com/blog/2016/12/26/arm-pointer-alignment-requirements/

  • if we want to use bdwgc later, we need to align pointer in stack

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then what about 32bit platform?
This patch marks alignment only for addresses in 64bit platform, but I think that this alignment needs to be applied for 32bit addresses too

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we don't consider alignment of 32-bit platform
Every value contents have multiple of 4-byte size and
and we have experience that alloca function returns aligned address

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I get it :)
Packing approach has this issue only for 64bit platform

Copy link
Collaborator

@clover2123 clover2123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@clover2123 clover2123 merged commit 0a6c695 into Samsung:interp Sep 6, 2023
12 checks passed
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