Skip to content

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

Open
gkgoat1 wants to merge 16 commits into
cfallin:mainfrom
portal-co:pr/changes1
Open

Support tail calls and bulk memory, optional function reordering pass#6
gkgoat1 wants to merge 16 commits into
cfallin:mainfrom
portal-co:pr/changes1

Conversation

@gkgoat1

@gkgoat1 gkgoat1 commented Jan 20, 2024

Copy link
Copy Markdown

These are some changes I find useful for WAFFLE

@cfallin

cfallin commented Jan 21, 2024

Copy link
Copy Markdown
Owner

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.

@cfallin cfallin left a comment

Copy link
Copy Markdown
Owner

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:

Comment thread src/backend/stackify.rs Outdated
Return {
values: &'a [Value],
},
ReturnCall {

Copy link
Copy Markdown
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?

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

Copy link
Copy Markdown
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?

Comment thread src/frontend.rs Outdated
t
);
if self.reachable {
// let values = values.to_vec();

Copy link
Copy Markdown
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?

Comment thread src/ir/module.rs Outdated
pub start_func: Option<Func>,
pub debug: Debug,
pub debug_map: DebugMap,
pub custom_sections: IndexMap<String,Vec<u8>>

Copy link
Copy Markdown
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...

Comment thread src/passes/reorder_funs.rs Outdated
Terminator, ValueDef,
};

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

Copy link
Copy Markdown
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
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, good point

Comment thread src/backend/mod.rs Outdated
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
Copy Markdown
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?

cfallin added a commit that referenced this pull request Jul 8, 2025
Add PrintDecorator hooks for Waffle's IR
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