Skip to content

Fix flambda_o3 and flambda_oclassic attributes #536

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
Merged
merged 8 commits into from
Feb 24, 2022
Merged

Fix flambda_o3 and flambda_oclassic attributes #536

merged 8 commits into from
Feb 24, 2022

Conversation

lukemaurer
Copy link
Contributor

@lukemaurer lukemaurer commented Feb 17, 2022 •
edited
Loading

The implementations of the flambda_o3 and flambda_oclassic attributes were calling into Clflags, but the flambda backend implements optimization flags by circumventing Clflags entirely. The only obvious solution is to add a hook to Clflags so we can simply override what Clflags.set_o3 () does. Fortunately, doing it this way means that the other two ways of setting the flags - the command line and OCAMLPARAM - no longer need to do anything special.

Tested manually using the included .ml source file.

@mshinwell mshinwell added flambda2 Prerequisite for, or part of, flambda2 flambda2 beta labels Feb 18, 2022
gretay-js
gretay-js reviewed Feb 18, 2022
View reviewed changes
@lukemaurer lukemaurer requested a review from poechsel as a code owner February 22, 2022 13:17
@lukemaurer lukemaurer requested a review from lthls as a code owner February 23, 2022 14:55
@mshinwell
Copy link
Collaborator

This is ok to merge once rebased + CI passes.

@lukemaurer
Fix flambda_o3 and flambda_oclassic attributes
58f575d 
The implementations of the `flambda_o3` and `flambda_oclassic`
attributes were calling into `Clflags`, but the flambda backend
implements optimization flags by circumventing `Clflags` entirely.
The only obvious solution is to add a hook to `Clflags` so we can
simply override what `Clflags.set_o3 ()` does. Fortunately, doing it
this way means that the other two ways of setting the flags - the
command line and `OCAMLPARAM` - no longer need to do anything
special.

Tested manually using the included .ml source file.

Note that `[@@@flambda_o3]` does nothing if `-Oclassic` is on. This
actually reflects existing behavior: `-Oclassic -O3` and `-O3
-Oclassic` are, as far as I can tell, effectively the same as
`-Oclassic`.
@lukemaurer
Make later -O flags completely override earlier ones
5d63b9a 
Also document the test more thoroughly.
@lukemaurer
Don't have -O* clobber earlier options
9c1454b 
Now `-flambda2-cse-depth 7 -O3` will do the same thing as
`-O3 -flambda2-cse-depth 7`.
@lukemaurer
Minimize the diff a bit
9f112fd
@lukemaurer
Install optimization-flag handler in native toplevel
3ea816e
@lukemaurer
Restore the behavior that -Oclassic implies -linscan
ac6409b
@lukemaurer
Add missing settings to O levels
d970748 
The explicit setting of `fallback_inlining_heuristic` to `false` in
the old `o2_flags` was redundant, but it's easier to review this way.
@lukemaurer
Formatting
2c18ae3
@mshinwell mshinwell merged commit e9030a1 into oxcaml:main Feb 24, 2022
mshinwell pushed a commit that referenced this pull request Mar 3, 2022
@lukemaurer @mshinwell
Fix flambda_o3 and flambda_oclassic attributes (#536)
af5bb3d 
* Fix flambda_o3 and flambda_oclassic attributes

The implementations of the `flambda_o3` and `flambda_oclassic`
attributes were calling into `Clflags`, but the flambda backend
implements optimization flags by circumventing `Clflags` entirely.
The only obvious solution is to add a hook to `Clflags` so we can
simply override what `Clflags.set_o3 ()` does. Fortunately, doing it
this way means that the other two ways of setting the flags - the
command line and `OCAMLPARAM` - no longer need to do anything
special.

Tested manually using the included .ml source file.

Note that `[@@@flambda_o3]` does nothing if `-Oclassic` is on. This
actually reflects existing behavior: `-Oclassic -O3` and `-O3
-Oclassic` are, as far as I can tell, effectively the same as
`-Oclassic`.

* Make later -O flags completely override earlier ones

Also document the test more thoroughly.

* Don't have -O* clobber earlier options

Now `-flambda2-cse-depth 7 -O3` will do the same thing as
`-O3 -flambda2-cse-depth 7`.

* Minimize the diff a bit

* Install optimization-flag handler in native toplevel

* Restore the behavior that -Oclassic implies -linscan

* Add missing settings to O levels

The explicit setting of `fallback_inlining_heuristic` to `false` in
the old `o2_flags` was redundant, but it's easier to review this way.

* Formatting
stedolan added a commit to stedolan/flambda-backend that referenced this pull request Mar 7, 2022
@stedolan
Squashed 'ocaml/' changes from 173842c..64235a3
c6bb9c5 
64235a3 flambda-backend: Change Float.nan from sNaN to qNaN (oxcaml#466)
14a8e27 flambda-backend: Track GC work for all managed bigarray allocations (upstream 11022) (oxcaml#569)
c3cda96 flambda-backend: Add two new methods to targetint for dwarf (oxcaml#560)
e6f1fed flambda-backend: Handle arithmetic overflow in select_addr (oxcaml#570)
dab7209 flambda-backend: Add Target_system to ocaml/utils (oxcaml#542)
82d5044 flambda-backend: Enhance numbers.ml with more primitive types (oxcaml#544)
216be99 flambda-backend: Fix flambda_o3 and flambda_oclassic attributes (oxcaml#536)
4b56e07 flambda-backend: Test naked pointer root handling (oxcaml#550)
40d69ce flambda-backend: Stop local function optimisation from moving code into function bodies; opaque_identity fixes for class compilation (oxcaml#537)
f08ae58 flambda-backend: Implemented inlining history and use it inside inlining reports (oxcaml#365)
ac496bf flambda-backend: Disable the local keyword in typing (oxcaml#540)
7d46712 flambda-backend: Bugfix for Typedtree generation of arrow types (oxcaml#539)
61a7b47 flambda-backend: Insert missing page table check in roots_nat.c (oxcaml#541)
323bd36 flambda-backend: Compiler error when -disable-all-extensions and -extension are used (oxcaml#534)
d8956b0 flambda-backend: Persistent environment and reproducibility (oxcaml#533)
4a0c89f flambda-backend: Revert "Revert bswap PRs (480 and 482)" (oxcaml#506)
7803705 flambda-backend: Cause a C warning when CAMLreturn is missing in C stubs. (oxcaml#376)
6199db5 flambda-backend: Improve unboxing during cmm for Flambda (oxcaml#295)
96b9e1b flambda-backend: Print diagnostics at runtime for Invalid (oxcaml#530)
42ab88e flambda-backend: Disable bytecode compilers in ocamltest (oxcaml#504)
58c72d5 flambda-backend: Backport ocaml/ocaml#10595 from upstream/trunk (oxcaml#471)
1010539 flambda-backend: Use C++ name mangling convention (oxcaml#483)
81881bb flambda-backend: Local allocation test no longer relies on lifting (oxcaml#525)
f5c4719 flambda-backend: Fix an assertion in Closure that breaks probes (oxcaml#505)
c2cf2b2 flambda-backend: Add some missing command line arguments to ocamlnat (oxcaml#499)

git-subtree-dir: ocaml
git-subtree-split: 64235a3
stedolan added a commit that referenced this pull request Mar 7, 2022
@stedolan
Squashed 'ocaml/' changes from 173842c..64235a3
b54d3a3 
64235a3 flambda-backend: Change Float.nan from sNaN to qNaN (#466)
14a8e27 flambda-backend: Track GC work for all managed bigarray allocations (upstream 11022) (#569)
c3cda96 flambda-backend: Add two new methods to targetint for dwarf (#560)
e6f1fed flambda-backend: Handle arithmetic overflow in select_addr (#570)
dab7209 flambda-backend: Add Target_system to ocaml/utils (#542)
82d5044 flambda-backend: Enhance numbers.ml with more primitive types (#544)
216be99 flambda-backend: Fix flambda_o3 and flambda_oclassic attributes (#536)
4b56e07 flambda-backend: Test naked pointer root handling (#550)
40d69ce flambda-backend: Stop local function optimisation from moving code into function bodies; opaque_identity fixes for class compilation (#537)
f08ae58 flambda-backend: Implemented inlining history and use it inside inlining reports (#365)
ac496bf flambda-backend: Disable the local keyword in typing (#540)
7d46712 flambda-backend: Bugfix for Typedtree generation of arrow types (#539)
61a7b47 flambda-backend: Insert missing page table check in roots_nat.c (#541)
323bd36 flambda-backend: Compiler error when -disable-all-extensions and -extension are used (#534)
d8956b0 flambda-backend: Persistent environment and reproducibility (#533)
4a0c89f flambda-backend: Revert "Revert bswap PRs (480 and 482)" (#506)
7803705 flambda-backend: Cause a C warning when CAMLreturn is missing in C stubs. (#376)
6199db5 flambda-backend: Improve unboxing during cmm for Flambda (#295)
96b9e1b flambda-backend: Print diagnostics at runtime for Invalid (#530)
42ab88e flambda-backend: Disable bytecode compilers in ocamltest (#504)
58c72d5 flambda-backend: Backport ocaml/ocaml#10595 from upstream/trunk (#471)
1010539 flambda-backend: Use C++ name mangling convention (#483)
81881bb flambda-backend: Local allocation test no longer relies on lifting (#525)
f5c4719 flambda-backend: Fix an assertion in Closure that breaks probes (#505)
c2cf2b2 flambda-backend: Add some missing command line arguments to ocamlnat (#499)

git-subtree-dir: ocaml
git-subtree-split: 64235a3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gretay-js gretay-js gretay-js left review comments

@mshinwell mshinwell mshinwell approved these changes

@poechsel poechsel poechsel approved these changes

@lthls lthls Awaiting requested review from lthls

Assignees
No one assigned
Labels
flambda2 beta flambda2 Prerequisite for, or part of, flambda2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lukemaurer @mshinwell @poechsel @gretay-js