-
Notifications
You must be signed in to change notification settings - Fork 830
Fix --spill-pointers #6294
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 --spill-pointers #6294
Conversation
This fixes --spill-pointers.
src/abi/stack.h
Outdated
| WASM_UNREACHABLE("unhandled pointerType"); | ||
| } | ||
| block->list.push_back(builder.makeGlobalSet(stackPointer->name, added)); | ||
| block->list.push_back(builder.makeLocalSet(local, added)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the cause of the error on CI, I think - the added node is reused here, but each node is only allowed to appear once in the IR.
The sub operation could be stored in another local but maybe there is a nicer way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the cause of the error on CI, I think - the
addednode is reused here, but each node is only allowed to appear once in the IR.
i didn't know that. thank you. fixed.
The sub operation could be stored in another local but maybe there is a nicer way.
i guess it can be something like:
global.get $sp
i32.const xxx
i32.sub
local.tee yyy
global.set $sp
but i'm not sure how it can be done with binaryen IR builder API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use builder.makeLocalTee to replace the makeLocalSet/Get pair here. That is, something like makeGlobalSet(makeLocalTee(..)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you. done.
The LLVM wasm backend grows the stack downwards, and this pass did not fully account for that before.
No description provided.