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.

Sign up for GitHub

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

Jump to bottom

Implement BinaryenOptimiser module #61

Merged
merged 5 commits into from
Sep 7, 2019
Merged

Implement BinaryenOptimiser module #61

merged 5 commits into from
Sep 7, 2019

Conversation

axic
Copy link
Member

@axic axic commented Jan 1, 2019

Closes #60.

@axic axic requested a review from jakelang January 1, 2019 11:35
axic
axic commented Jan 1, 2019
View reviewed changes
libchisel/src/binaryenopt.rs Outdated Show resolved Hide resolved
@jakelang
Copy link
Collaborator

jakelang commented Jan 2, 2019

@axic need to fix CI's dependency issue.

jakelang
jakelang reviewed Jan 2, 2019
View reviewed changes
libchisel/Cargo.toml Outdated Show resolved Hide resolved
jakelang
jakelang previously requested changes Jan 2, 2019
View reviewed changes
Copy link
Collaborator

@jakelang jakelang left a comment

Choose a reason for hiding this comment

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

Needs documentation in README.

libchisel/src/binaryenopt.rs
use super::{ModuleError, ModulePreset, ModuleTranslator};
use parity_wasm::elements::*;

// FIXME: change level names
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps something renaming the enum itself would be clean enough. Something like BinaryenOptimizationLevel::O1, etc. would be more clear, I think.

libchisel/src/binaryenopt.rs Show resolved Hide resolved
libchisel/src/binaryenopt.rs Outdated Show resolved Hide resolved
libchisel/src/binaryenopt.rs Outdated Show resolved Hide resolved
libchisel/src/binaryenopt.rs Outdated Show resolved Hide resolved
libchisel/src/binaryenopt.rs Outdated Show resolved Hide resolved
libchisel/src/binaryenopt.rs
}

fn translate(&self, module: &Module) -> Result<Module, ModuleError> {
// FIXME: could just move this into `BinaryenOptimiser`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, would be much cleaner.

@jakelang jakelang added the wip label Jan 2, 2019
@jakelang
Copy link
Collaborator

jakelang commented Jan 2, 2019

needs rebase.

@axic axic force-pushed the binaryen branch 2 times, most recently from 6e61ff3 to fe2751d Compare January 4, 2019 02:50
@axic
Copy link
Member Author

axic commented Jan 9, 2019 •
edited
Loading

@jakelang this is not final (tests aren't updated), but some things have changed, please have a look again

@axic axic force-pushed the binaryen branch 3 times, most recently from c187717 to c744590 Compare January 26, 2019 01:18
jakelang
jakelang reviewed Jan 26, 2019
View reviewed changes
chisel/src/main.rs Outdated Show resolved Hide resolved
@axic
Copy link
Member Author

axic commented Jan 26, 2019 

   Compiling binaryen-sys v0.6.0
error: failed to run custom build command for `binaryen-sys v0.6.0`
process didn't exit successfully: `/root/project/target/release/build/binaryen-sys-2b0f3bd75d10d05b/build-script-build` (exit code: 101)
--- stderr
fatal: Not a git repository (or any of the parent directories): .git
thread 'main' panicked at 'Unable to find libclang: "couldn\'t find any of [\'libclang.so\', \'libclang.so.*\'], set the LIBCLANG_PATH environment variable to a path where one of these files can be found (skipped: [])"', src/libcore/result.rs:1009:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.

@pepyakin any idea why CI would fail to build it?

@pepyakin
Copy link

Oh looks like libclang isn't installed and binaryen-rs depends on bindgen (for now) which uses libclang. I think installing clang will solve the issue

@axic
Copy link
Member Author

axic commented Jan 26, 2019

Ah right, that might be it, will try. Would be so much nicer cargo could scream when bindgen is missing a non-rust dependency :)

@axic axic force-pushed the binaryen branch 4 times, most recently from 51da914 to bf1cd51 Compare January 31, 2019 18:09
@axic axic force-pushed the binaryen branch 5 times, most recently from bc423b7 to 1ca7cfe Compare May 13, 2019 21:31
@axic axic requested a review from jakelang June 4, 2019 09:43
@axic axic removed the wip label Jun 4, 2019
@jakelang jakelang added enhancement New feature or request ready Ready for review and merge labels Jun 4, 2019
@axic axic force-pushed the binaryen branch 3 times, most recently from e376ab8 to bc1ac21 Compare June 6, 2019 13:38
@axic axic force-pushed the binaryen branch 5 times, most recently from 1a88715 to 31d31b7 Compare July 16, 2019 13:16
@axic
Copy link
Member Author

axic commented Sep 7, 2019

This works finally, including the CI. Crazy.

@axic axic merged commit 83133cf into master Sep 7, 2019
@axic axic deleted the binaryen branch September 7, 2019 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pepyakin pepyakin pepyakin left review comments

@jakelang jakelang jakelang approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request ready Ready for review and merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support optimisations from binaryen
3 participants
@axic @jakelang @pepyakin