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

Support tail calls and bulk memory, optional function reordering pass #6

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

gkgoat1
Copy link

@gkgoat1 gkgoat1 commented Jan 20, 2024

These are some changes I find useful for WAFFLE

@cfallin
Copy link
Owner

cfallin commented Jan 21, 2024

Thanks for this PR, @gkgoat1! I'm currently on parental leave and not writing or reviewing code, but I will take a look at this when I am back in mid-February.

Copy link
Owner

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Thanks @gkgoat1 -- some comments below:

Return {
values: &'a [Value],
},
ReturnCall {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add doc-comments to ReturnCall and ReturnCallIndirect variants here?

src/frontend.rs Outdated
type_index,
table_index,
} => {
// let sig = self.module.funcs[Func::new(*function_index as usize)].sig();
Copy link
Owner

Choose a reason for hiding this comment

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

Can you remove the commented-out line here?

src/frontend.rs Outdated
t
);
if self.reachable {
// let values = values.to_vec();
Copy link
Owner

Choose a reason for hiding this comment

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

Can you remove the commented-out line of code here?

src/ir/module.rs Outdated
@@ -19,6 +20,7 @@ pub struct Module<'a> {
pub start_func: Option<Func>,
pub debug: Debug,
pub debug_map: DebugMap,
pub custom_sections: IndexMap<String,Vec<u8>>
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind running cargo fmt on your changes? This appears not to be rustfmt'd...

Terminator, ValueDef,
};

pub fn reorder_funcs_in_body(b: &mut FunctionBody, f: &BTreeMap<Func, Func>) {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you describe the goal of this pass? To me it reads like a possibly useful utility, but not a core optimization pass that a general compiler backend would have -- would it perhaps work just as well to host it in your use-case directly?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, good point

@@ -763,6 +788,9 @@ pub fn compile(module: &Module<'_>) -> anyhow::Result<Vec<u8>> {
}
names.functions(&func_names);
into_mod.section(&names);
for (k, v) in module.custom_sections.iter() {
into_mod.section(&CustomSection { name: &k, data: &v });
Copy link
Owner

Choose a reason for hiding this comment

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

I definitely understand the usefulness of preserving custom sections, but my main concern here is that some custom sections will refer to code or other contents of the module that we will have modified (or completely regenerated), and will no longer be valid. For example, a debug (DWARF info) section will no longer have valid offsets, and will at least confuse, if not crash, a debugger that tries to use it in conjunction with our output.

Is there a particular kind of custom section you need to preserve? Perhaps we could have an allow-list of custom sections that remain valid and thus are preserved?

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.

3 participants