Skip to content

Commit

Permalink
feat: implement -o flags for disabling optimizations (#5385)
Browse files Browse the repository at this point in the history
## Description
This PR implements `-o` flags so that we can disable optimizations. This
will be useful as we are working towards debugger support and
optimizations pollute source map generated.

Provide option to the e2e test binary to specify build profile. For
tests that have their ABI tested, or rely on their compiled
hashes (for deployment), since that changes with build profile, the
tests are marked as unsupported for debug profile.
(better to test with and have expected results for release than the
debug profile).

Add testing release profile in CI since the default is now debug. Two
tests in `should_fail` that are expected to fail due to
overflows are disabled because there's a bug in our IR gen. This is 
tracked by #5449 

---------

Co-authored-by: Vaivaswatha Nagaraj <vaivaswatha.nagaraj@fuel.sh>
Co-authored-by: Vaivaswatha N <vaivaswatha@users.noreply.github.com>
Co-authored-by: Sophie Dankel <47993817+sdankel@users.noreply.github.com>
  • Loading branch information
4 people committed Jan 20, 2024
1 parent 2ac7030 commit ef7a7e4
Show file tree
Hide file tree
Showing 81 changed files with 248 additions and 152 deletions.
20 changes: 19 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,24 @@ jobs:
- name: Cargo Run E2E Tests (Fuel VM)
run: cargo run --locked --release --bin test -- --locked

cargo-run-e2e-test-release:
runs-on: ubuntu-latest
needs: get-fuel-core-version
services:
fuel-core:
image: ghcr.io/fuellabs/fuel-core:v${{ needs.get-fuel-core-version.outputs.fuel_core_version }}
ports:
- 4000:4000
steps:
- uses: actions/checkout@v3
- name: Install toolchain
uses: dtolnay/rust-toolchain@master
with:
toolchain: ${{ env.RUST_VERSION }}
- uses: Swatinem/rust-cache@v2
- name: Cargo Run E2E Tests (Fuel VM)
run: cargo run --locked --release --bin test -- --locked --release

cargo-run-e2e-test-evm:
runs-on: ubuntu-latest
steps:
Expand Down Expand Up @@ -349,7 +367,7 @@ jobs:
toolchain: ${{ env.RUST_VERSION }}
- uses: Swatinem/rust-cache@v2
- name: Build All Tests
run: cargo run --locked -p forc -- build --locked --path ./test/src/sdk-harness
run: cargo run --locked --release -p forc -- build --release --locked --path ./test/src/sdk-harness
- name: Cargo Test sway-lib-std
run: cargo test --locked --release --manifest-path ./test/src/sdk-harness/Cargo.toml -- --nocapture

Expand Down
7 changes: 6 additions & 1 deletion forc-pkg/src/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ use std::{
path::{Path, PathBuf},
sync::Arc,
};
use sway_core::{fuel_prelude::fuel_tx, language::parsed::TreeType, parse_tree_type, BuildTarget};
use sway_core::{
fuel_prelude::fuel_tx, language::parsed::TreeType, parse_tree_type, BuildTarget, OptLevel,
};
use sway_error::handler::Handler;
use sway_utils::{
constants, find_nested_manifest_dir, find_parent_manifest_dir,
Expand Down Expand Up @@ -233,6 +235,7 @@ pub struct BuildProfile {
#[serde(default)]
pub error_on_warnings: bool,
pub reverse_results: bool,
pub optimization_level: OptLevel,
#[serde(default)]
pub experimental: ExperimentalFlags,
}
Expand Down Expand Up @@ -726,6 +729,7 @@ impl BuildProfile {
json_abi_with_callpaths: false,
error_on_warnings: false,
reverse_results: false,
optimization_level: OptLevel::Opt0,
experimental: ExperimentalFlags {
new_encoding: false,
},
Expand All @@ -747,6 +751,7 @@ impl BuildProfile {
json_abi_with_callpaths: false,
error_on_warnings: false,
reverse_results: false,
optimization_level: OptLevel::Opt1,
experimental: ExperimentalFlags {
new_encoding: false,
},
Expand Down
1 change: 1 addition & 0 deletions forc-pkg/src/pkg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1564,6 +1564,7 @@ pub fn sway_build_config(
.with_include_tests(build_profile.include_tests)
.with_time_phases(build_profile.time_phases)
.with_metrics(build_profile.metrics_outfile.clone())
.with_optimization_level(build_profile.optimization_level)
.with_experimental(sway_core::ExperimentalFlags {
new_encoding: build_profile.experimental.new_encoding,
});
Expand Down
15 changes: 15 additions & 0 deletions sway-core/src/build_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ pub enum BuildTarget {
MidenVM,
}

#[derive(Serialize, Deserialize, Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub enum OptLevel {
Opt0,
Opt1,
}

/// Configuration for the overall build and compilation process.
#[derive(Clone)]
pub struct BuildConfig {
Expand All @@ -46,6 +52,7 @@ pub struct BuildConfig {
pub(crate) print_finalized_asm: bool,
pub(crate) print_ir: bool,
pub(crate) include_tests: bool,
pub(crate) optimization_level: OptLevel,
pub time_phases: bool,
pub metrics_outfile: Option<String>,
pub experimental: ExperimentalFlags,
Expand Down Expand Up @@ -93,6 +100,7 @@ impl BuildConfig {
include_tests: false,
time_phases: false,
metrics_outfile: None,
optimization_level: OptLevel::Opt0,
experimental: ExperimentalFlags::default(),
}
}
Expand Down Expand Up @@ -146,6 +154,13 @@ impl BuildConfig {
}
}

pub fn with_optimization_level(self, optimization_level: OptLevel) -> Self {
Self {
optimization_level,
..self
}
}

/// Whether or not to include test functions in parsing, type-checking and codegen.
///
/// This should be set to `true` by invocations like `forc test` or `forc check --tests`.
Expand Down
33 changes: 25 additions & 8 deletions sway-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use crate::source_map::SourceMap;
pub use asm_generation::from_ir::compile_ir_to_asm;
use asm_generation::FinalizedAsm;
pub use asm_generation::{CompiledBytecode, FinalizedEntry};
pub use build_config::{BuildConfig, BuildTarget};
pub use build_config::{BuildConfig, BuildTarget, OptLevel};
use control_flow_analysis::ControlFlowGraph;
use metadata::MetadataManager;
use query_engine::{ModuleCacheKey, ModulePath, ProgramsCacheEntry};
Expand All @@ -40,9 +40,10 @@ use std::sync::Arc;
use sway_ast::AttributeDecl;
use sway_error::handler::{ErrorEmitted, Handler};
use sway_ir::{
create_o1_pass_group, register_known_passes, Context, Kind, Module, PassManager,
ARGDEMOTION_NAME, CONSTDEMOTION_NAME, DCE_NAME, MEM2REG_NAME, MEMCPYOPT_NAME,
MISCDEMOTION_NAME, MODULEPRINTER_NAME, RETDEMOTION_NAME, SIMPLIFYCFG_NAME, SROA_NAME,
create_o1_pass_group, register_known_passes, Context, Kind, Module, PassGroup, PassManager,
ARGDEMOTION_NAME, CONSTDEMOTION_NAME, DCE_NAME, INLINE_MODULE_NAME, MEM2REG_NAME,
MEMCPYOPT_NAME, MISCDEMOTION_NAME, MODULEPRINTER_NAME, RETDEMOTION_NAME, SIMPLIFYCFG_NAME,
SROA_NAME,
};
use sway_types::constants::DOC_COMMENT_ATTRIBUTE_NAME;
use sway_types::SourceEngine;
Expand Down Expand Up @@ -804,7 +805,17 @@ pub(crate) fn compile_ast_to_ir_to_asm(
// Initialize the pass manager and register known passes.
let mut pass_mgr = PassManager::default();
register_known_passes(&mut pass_mgr);
let mut pass_group = create_o1_pass_group();
let mut pass_group = PassGroup::default();

match build_config.optimization_level {
OptLevel::Opt1 => {
pass_group.append_group(create_o1_pass_group());
}
OptLevel::Opt0 => {
// Inlining is necessary until #4899 is resolved.
pass_group.append_pass(INLINE_MODULE_NAME);
}
}

// Target specific transforms should be moved into something more configured.
if build_config.build_target == BuildTarget::Fuel {
Expand All @@ -823,9 +834,15 @@ pub(crate) fn compile_ast_to_ir_to_asm(
// Run a DCE and simplify-cfg to clean up any obsolete instructions.
pass_group.append_pass(DCE_NAME);
pass_group.append_pass(SIMPLIFYCFG_NAME);
pass_group.append_pass(SROA_NAME);
pass_group.append_pass(MEM2REG_NAME);
pass_group.append_pass(DCE_NAME);

match build_config.optimization_level {
OptLevel::Opt1 => {
pass_group.append_pass(SROA_NAME);
pass_group.append_pass(MEM2REG_NAME);
pass_group.append_pass(DCE_NAME);
}
OptLevel::Opt0 => {}
}
}

if build_config.print_ir {
Expand Down
6 changes: 6 additions & 0 deletions test/src/e2e_vm_tests/harness.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use anyhow::{anyhow, bail, Result};
use colored::Colorize;
use forc::cli::shared::BuildProfile;
use forc_client::{
cmd::{Deploy as DeployCommand, Run as RunCommand},
op::{deploy, run},
Expand Down Expand Up @@ -72,6 +73,10 @@ pub(crate) async fn deploy_contract(file_name: &str, run_config: &RunConfig) ->
},
signing_key: Some(SecretKey::from_str(SECRET_KEY).unwrap()),
default_salt: true,
build_profile: BuildProfile {
release: run_config.release,
..Default::default()
},
..Default::default()
})
.await
Expand Down Expand Up @@ -252,6 +257,7 @@ pub(crate) async fn compile_to_bytes(file_name: &str, run_config: &RunConfig) ->
let manifest_dir = env!("CARGO_MANIFEST_DIR");
let build_opts = forc_pkg::BuildOpts {
build_target: run_config.build_target,
release: run_config.release,
pkg: forc_pkg::PkgOpts {
path: Some(format!(
"{manifest_dir}/src/e2e_vm_tests/test_programs/{file_name}",
Expand Down
29 changes: 29 additions & 0 deletions test/src/e2e_vm_tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use anyhow::{anyhow, bail, Result};
use assert_matches::assert_matches;
use colored::*;
use core::fmt;
use forc_pkg::BuildProfile;
use fuel_vm::fuel_tx;
use fuel_vm::prelude::*;
use regex::Regex;
Expand Down Expand Up @@ -68,6 +69,7 @@ struct TestDescription {
validate_abi: bool,
validate_storage_slots: bool,
supported_targets: HashSet<BuildTarget>,
unsupported_profiles: Vec<&'static str>,
checker: filecheck::Checker,
}

Expand Down Expand Up @@ -539,6 +541,12 @@ pub async fn run(filter_config: &FilterConfig, run_config: &RunConfig) -> Result
if filter_config.first_only && !tests.is_empty() {
tests = vec![tests.remove(0)];
}
let cur_profile = if run_config.release {
BuildProfile::RELEASE
} else {
BuildProfile::DEBUG
};
tests.retain(|t| !t.unsupported_profiles.contains(&cur_profile));

// Run tests
let context = TestContext {
Expand Down Expand Up @@ -866,6 +874,15 @@ fn parse_test_toml(path: &Path) -> Result<TestDescription> {
.map(get_test_abi_from_value)
.collect::<Result<Vec<BuildTarget>>>()?;

// Check for not supported build profiles. Default is empty.
let unsupported_profiles = toml_content
.get("unsupported_profiles")
.map(|v| v.as_array().cloned().unwrap_or_default())
.unwrap_or_default()
.iter()
.map(get_build_profile_from_value)
.collect::<Result<Vec<&'static str>>>()?;

let supported_targets = HashSet::from_iter(if supported_targets.is_empty() {
vec![BuildTarget::Fuel]
} else {
Expand All @@ -883,6 +900,7 @@ fn parse_test_toml(path: &Path) -> Result<TestDescription> {
validate_abi,
validate_storage_slots,
supported_targets,
unsupported_profiles,
checker,
})
}
Expand All @@ -897,6 +915,17 @@ fn get_test_abi_from_value(value: &toml::Value) -> Result<BuildTarget> {
}
}

fn get_build_profile_from_value(value: &toml::Value) -> Result<&'static str> {
match value.as_str() {
Some(profile) => match profile {
BuildProfile::DEBUG => Ok(BuildProfile::DEBUG),
BuildProfile::RELEASE => Ok(BuildProfile::RELEASE),
_ => Err(anyhow!(format!("Unknown build profile"))),
},
None => Err(anyhow!("Invalid TOML value")),
}
}

fn get_expected_result(toml_content: &toml::Value) -> Result<TestResult> {
fn get_action_value(action: &toml::Value, expected_value: &toml::Value) -> Result<TestResult> {
match (action.as_str(), expected_value) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
category = "run"
category = "disabled"
expected_result = { action = "revert", value = 0 }
validate_abi = false
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
category = "run"
category = "disabled"
expected_result = { action = "revert", value = 0 }
validate_abi = false
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ category = "run"
expected_result = { action = "return", value = 0 }
validate_abi = true
expected_warnings = 2
unsupported_profiles = ["debug"]
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ fn reference_local_reference_var_and_value_not_inlined<T>()
}

#[inline(always)]
fn reference_zero_sized_local_var_and_value<T>(is_inlined: bool)
fn reference_zero_sized_local_var_and_value<T>()
where T: Eq + New + ZeroSize
{
assert(__size_of::<T>() == 0);
Expand All @@ -127,21 +127,6 @@ fn reference_zero_sized_local_var_and_value<T>(is_inlined: bool)
let r_val_ptr = asm(r: r_val) { r: raw_ptr };
let r_dummy_ptr = asm(r: r_dummy) { r: raw_ptr };

// If there is no inlining and mixing with other test functions,
// since the size of `T` is zero, means allocates zero memory,
// both created values will be on the same memory location.
// The dummy value will also be on the same location.
// In case of inlining with other test functions we can get
// the two variables position separately from each other, intermixed
// with the locals coming from other functions.
// Note that we rely on the optimization which will remove the local
// variables for the references. This assumption is fine,
// the test should never be flaky.
if (!is_inlined) {
assert(r_x_1_ptr == r_val_ptr);
assert(r_x_1_ptr == r_dummy_ptr);
}

assert(r_x_1_ptr == r_x_2_ptr);

let r_x_1_ptr_val = r_x_1_ptr.read::<T>();
Expand All @@ -164,7 +149,7 @@ fn reference_zero_sized_local_var_and_value<T>(is_inlined: bool)
fn reference_zero_sized_local_var_and_value_not_inlined<T>()
where T: Eq + New + ZeroSize
{
reference_zero_sized_local_var_and_value::<T>(false)
reference_zero_sized_local_var_and_value::<T>()
}

#[inline(never)]
Expand All @@ -186,8 +171,8 @@ fn test_all_inlined() {
reference_local_var_and_value::<raw_ptr>();
reference_local_var_and_value::<raw_slice>();

reference_zero_sized_local_var_and_value::<EmptyStruct>(true);
reference_zero_sized_local_var_and_value::<[u64;0]>(true);
reference_zero_sized_local_var_and_value::<EmptyStruct>();
reference_zero_sized_local_var_and_value::<[u64;0]>();

reference_local_reference_var_and_value::<()>();
reference_local_reference_var_and_value::<bool>();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
category = "run"
expected_result = { action = "return_data", value = "0000000000000000000000000000000000000000000000000000000000000001" }
validate_abi = true
unsupported_profiles = ["debug"]
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
category = "run_on_node"
expected_result = { action = "result", value = 1 }
contracts = ["should_pass/test_contracts/array_of_structs_contract"]
unsupported_profiles = ["debug"]
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
category = "run_on_node"
expected_result = { action = "result", value = 1 }
contracts = ["should_pass/test_contracts/balance_test_contract", "should_pass/test_contracts/test_fuel_coin_contract"]
unsupported_profiles = ["debug"]
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
category = "run_on_node"
expected_result = { action = "result", value = 1 }
contracts = ["should_pass/test_contracts/balance_test_contract"]
unsupported_profiles = ["debug"]
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
category = "run_on_node"
expected_result = { action = "result", value = 1 }
contracts = ["should_pass/test_contracts/abi_with_tuples_contract"]
unsupported_profiles = ["debug"]
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
category = "run_on_node"
expected_result = { action = "result", value = 4242 }
contracts = ["should_pass/test_contracts/basic_storage"]
unsupported_profiles = ["debug"]
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
category = "run_on_node"
expected_result = { action = "result", value = 0 }
contracts = ["should_pass/test_contracts/contract_with_type_aliases"]
unsupported_profiles = ["debug"]
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
category = "run_on_node"
expected_result = { action = "result", value = 1 }
contracts = ["should_pass/test_contracts/increment_contract"]
unsupported_profiles = ["debug"]
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
category = "run_on_node"
expected_result = { action = "result", value = 171 }
contracts = ["should_pass/test_contracts/storage_enum_contract"]
unsupported_profiles = ["debug"]
Loading

0 comments on commit ef7a7e4

Please sign in to comment.