feat: implement `-o` flags for disabling optimizations #5385

kayagokalp · 2023-12-12T22:06:38Z

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. A new Issue will be opened for this.

kayagokalp · 2023-12-15T18:13:17Z

After discussing it with @vaivaswatha, this is blocked as some optimizations are currently needed for producing correct output, so we cannot directly chug them depending on the o flag, we will have to be more careful about that.

vaivaswatha · 2024-01-04T05:28:15Z

@kayagokalp I've updated the PR to disable as many optimizations as possible on -O0 (which is the default path). Can you check if the location information has improved for you with this?

The testsuite mostly passes. The ones failing are related to changed ABIs or checksums (for deployment ones).

This brings us to the task of supporting two paths in our testsuite, i.e., testing both -O0 and -O1, which I believe should be part of this PR. We can get to it after you confirm that disabling some of these optimizations indeed did help you.

kayagokalp · 2024-01-04T11:21:30Z

Thanks for the help @vaivaswatha I will be reporting before and after changes here today 🙌

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. A new Issue will be opened for this.

The test now no longer makes assumptions about addresses of zero sized values.

vaivaswatha · 2024-01-17T05:43:03Z

Thanks for the help @vaivaswatha I will be reporting before and after changes here today 🙌

@kayagokalp does having fewer optimizations improve the quality of source location information?

.github/workflows/ci.yml

forc-pkg/src/manifest.rs

test/src/e2e_vm_tests/test_programs/should_fail/arith_overflow/u8_add_overflow/test.toml

...ts/test_programs/should_pass/language/references/referencing_local_vars_and_values/test.toml

sdankel

Looks good! Thank you!!

vaivaswatha · 2024-01-19T04:49:13Z

@tritao if this is ok, except for #5486 related changes, we can merge it and update that PR. Or we can wait till that is merged.

tritao · 2024-01-19T12:17:26Z

@tritao if this is ok, except for #5486 related changes, we can merge it and update that PR. Or we can wait till that is merged.

Cool by me but that PR was already merged anyway

…yagokalp/o-flags

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>

kayagokalp added enhancement New feature or request compiler General compiler. Should eventually become more specific as the issue is triaged forc forc-pkg Everything related to the `forc-pkg` crate. labels Dec 12, 2023

kayagokalp self-assigned this Dec 12, 2023

feat: implement o flags for disabling optimizations

41d7d0d

kayagokalp force-pushed the kayagokalp/o-flags branch from 2d8a924 to 41d7d0d Compare December 14, 2023 16:50

kayagokalp added the blocked label Dec 15, 2023

kayagokalp added the compiler: ir IRgen and sway-ir including optimization passes label Dec 15, 2023

vaivaswatha self-assigned this Jan 1, 2024

vaivaswatha added 2 commits January 1, 2024 13:44

Merge branch 'master' into kayagokalp/o-flags

93b0e3d

Disable some optimizations on -O0

2b57396

Merge branch 'master' into kayagokalp/o-flags

81dc8d6

vaivaswatha mentioned this pull request Jan 10, 2024

Overflow behaviour relies on Optimization #5449

Closed

vaivaswatha and others added 2 commits January 10, 2024 20:36

Merge branch 'master' into kayagokalp/o-flags

3b453b5

Disable referencing_local_vars_and_values test

3d2a3c7

vaivaswatha mentioned this pull request Jan 13, 2024

Test referencing_local_vars_and_values fails without optimizations #5465

Closed

vaivaswatha and others added 5 commits January 13, 2024 10:39

Merge branch 'master' into kayagokalp/o-flags

73646b3

Renable referencing_local_vars_and_values test.

6ceede2

The test now no longer makes assumptions about addresses of zero sized values.

Update fuel-core version in CI change

a21e36a

Merge remote-tracking branch 'origin/master' into kayagokalp/o-flags

26950ae

Fix typo during merge

776d695

vaivaswatha and others added 3 commits January 18, 2024 11:42

e2e testing: deployed contracts must use the right build profile

eb628d2

Merge branch 'master' into kayagokalp/o-flags

54c6929

Testing: build sway-lib-std in release mode

6b1b48f

vaivaswatha added 3 commits January 18, 2024 14:25

Run release version of forc for sway-lib-std test

52d0db1

Update sdk-harness abi path to release

4896aaf

Update sdk-harness abi path to release missed out in previous commit

c5fe9ef

vaivaswatha marked this pull request as ready for review January 18, 2024 12:05

vaivaswatha requested a review from a team January 18, 2024 14:04

tritao reviewed Jan 18, 2024

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

tritao reviewed Jan 18, 2024

View reviewed changes

forc-pkg/src/manifest.rs Outdated Show resolved Hide resolved

tritao reviewed Jan 18, 2024

View reviewed changes

test/src/e2e_vm_tests/test_programs/should_fail/arith_overflow/u8_add_overflow/test.toml Show resolved Hide resolved

tritao reviewed Jan 18, 2024

View reviewed changes

...ts/test_programs/should_pass/language/references/referencing_local_vars_and_values/test.toml Outdated Show resolved Hide resolved

vaivaswatha and others added 2 commits January 19, 2024 09:29

attend to review comments

9e0fda9

Merge branch 'master' into kayagokalp/o-flags

89cc106

sdankel previously approved these changes Jan 19, 2024

View reviewed changes

Merge branch 'master' into kayagokalp/o-flags

0537073

sdankel mentioned this pull request Jan 19, 2024

Adds a DAP server with support for debugging sway unit tests #5477

Merged

7 tasks

vaivaswatha added 2 commits January 19, 2024 19:19

Merge branch 'master' into kayagokalp/o-flags

ca332a2

Merge branch 'kayagokalp/o-flags' of github.com:FuelLabs/sway into ka…

9bc886d

…yagokalp/o-flags

vaivaswatha dismissed sdankel’s stale review via 9bc886d January 19, 2024 13:50

Fix missing requires in CI

df2be76

tritao approved these changes Jan 19, 2024

View reviewed changes

vaivaswatha requested a review from a team January 19, 2024 14:08

vaivaswatha enabled auto-merge (squash) January 19, 2024 14:09

Merge branch 'master' into kayagokalp/o-flags

a1de5bc

sdankel approved these changes Jan 20, 2024

View reviewed changes

vaivaswatha merged commit ef7a7e4 into master Jan 20, 2024
37 checks passed

vaivaswatha deleted the kayagokalp/o-flags branch January 20, 2024 11:12

This was referenced Jan 24, 2024

Make debug BuildProfile actually skip compiler optimizations #5339

Closed

Source location / debug info for actually dead source code #5511

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement `-o` flags for disabling optimizations #5385

feat: implement `-o` flags for disabling optimizations #5385

kayagokalp commented Dec 12, 2023 •

edited by vaivaswatha

Loading

kayagokalp commented Dec 15, 2023

vaivaswatha commented Jan 4, 2024 •

edited

Loading

kayagokalp commented Jan 4, 2024

vaivaswatha commented Jan 17, 2024

sdankel left a comment

vaivaswatha commented Jan 19, 2024

tritao commented Jan 19, 2024

feat: implement -o flags for disabling optimizations #5385

feat: implement -o flags for disabling optimizations #5385

Conversation

kayagokalp commented Dec 12, 2023 • edited by vaivaswatha Loading

Description

kayagokalp commented Dec 15, 2023

vaivaswatha commented Jan 4, 2024 • edited Loading

kayagokalp commented Jan 4, 2024

vaivaswatha commented Jan 17, 2024

sdankel left a comment

Choose a reason for hiding this comment

vaivaswatha commented Jan 19, 2024

tritao commented Jan 19, 2024

feat: implement `-o` flags for disabling optimizations #5385

feat: implement `-o` flags for disabling optimizations #5385

kayagokalp commented Dec 12, 2023 •

edited by vaivaswatha

Loading

vaivaswatha commented Jan 4, 2024 •

edited

Loading