Add compiler flag to disable inlining

[javra]

For the discussion see the issue. Adds fields disable_inlining to CompilerConfig, RootDatabaseBuilder and OptimizationConfig. I don't know if there is any slicker way of adding the option.

This change is 

[orizi]

Reviewed 6 of 8 files at r1, all commit messages.
Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion (waiting on @javra)

---

crates/cairo-lang-lowering/src/inline/mod.rs line 68 at r1 (raw file):

    )?;

    Ok(match config {

Move the query out of the match.
Ok(if db.optconfig().disable..() { matches!(config, Always(_)) } else .....

[mkaput]

Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @javra and @orizi)

---

crates/bin/cairo-compile/src/main.rs line 27 at r1 (raw file):

    /// Disables inlining functions
    #[arg(short, long, default_value_t = false)]
    disable_inlining: bool,

I don't feel this is a good representation of the property here:

1. It's negated.
2. I feel true/false may not be enough cases.

What do you think about using an enum here and in CompilerConfig? Say

#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
enum IniliningStrategy {
    #[default]
    Default,
    Avoid, // Future case I can imagine, allow inline(always) but forbid others.
    Forbid
}

[mkaput]

Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @javra and @orizi)

---

crates/bin/cairo-compile/src/main.rs line 27 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I don't feel this is a good representation of the property here:

1. It's negated.
2. I feel true/false may not be enough cases.

What do you think about using an enum here and in CompilerConfig? Say

#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
enum IniliningStrategy {
    #[default]
    Default,
    Avoid, // Future case I can imagine, allow inline(always) but forbid others.
    Forbid
}

Oh, just noticed there's InliningConfiguration enum already in the codebase -- reuse it

[javra]

Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @javra and @orizi)

crates/bin/cairo-compile/src/main.rs line 27 at r1 (raw file):

    /// Disables inlining functions
    #[arg(short, long, default_value_t = false)]
    disable_inlining: bool,

I don't feel this is a good representation of the property here:

1. It's negated.

2. I feel `true`/`false` may not be enough cases.

What do you think about using an enum here and in CompilerConfig? Say

#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
enum IniliningStrategy {
    #[default]
    Default,
    Avoid, // Future case I can imagine, allow inline(always) but forbid others.
    Forbid
}

Sounds good, like I said in the other discussions, I'd love an option that overrides even the local inline(always) annotations.

[javra]

Oh, just noticed there's InliningConfiguration enum already in the codebase -- reuse it

Do the same four options in InlineConfiguration make sense as global options as well though? How to reconcile the local vs the global ones then? 🤔

[mkaput]

@javra please use Reviewable in this repository for responding to comments, you can find the button in PR description :)

Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @javra and @orizi)

[mkaput]

Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @javra and @orizi)

---

crates/bin/cairo-compile/src/main.rs line 27 at r1 (raw file):

Previously, javra (Jakob von Raumer) wrote…

Sounds good, like I said in the other discussions, I'd love an option that overrides even the local inline(always) annotations.

Ah you're right, it's not exactly representing what we're conveying here. So my first comment applies then

[javra]

Reviewable status: 0 of 12 files reviewed, 3 unresolved discussions (waiting on @mkaput and @orizi)

---

crates/cairo-lang-lowering/src/utils.rs line 14 at r2 (raw file):

/// Options for the `inlining` arguments
#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, clap::ValueEnum)]
pub enum InliningStrategy {

I put the enum in cairo-lang-lowering::utils, maybe cairo-lang-utils might be the better choice?

[mkaput]

Reviewable status: 0 of 12 files reviewed, 2 unresolved discussions (waiting on @javra and @orizi)

---

crates/cairo-lang-lowering/src/utils.rs line 14 at r2 (raw file):

Previously, javra (Jakob von Raumer) wrote…

I put the enum in cairo-lang-lowering::utils, maybe cairo-lang-utils might be the better choice?

I think lowering is the good place, but using clap in lib-crates is not.

It'd be best IMO to:

1. copy-paste this enum just with clap::ValueEnum to cairo-compile
2. add there the following:

impl From<crate::InliningStrategy> for cairo_lang_lowering::InliningStrategy {
...
}

[orizi]

Reviewed 5 of 12 files at r2, all commit messages.
Reviewable status: 5 of 12 files reviewed, 4 unresolved discussions (waiting on @javra and @mkaput)

---

crates/cairo-lang-lowering/Cargo.toml line 19 at r3 (raw file):

cairo-lang-syntax = { path = "../cairo-lang-syntax", version = "~2.6.4" }
cairo-lang-utils = { path = "../cairo-lang-utils", version = "~2.6.4" }
clap.workspace = true

this shouldn't be a dependency of the lowering.

---

crates/cairo-lang-lowering/src/utils.rs line 14 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I think lowering is the good place, but using clap in lib-crates is not.

It'd be best IMO to:

1. copy-paste this enum just with clap::ValueEnum to cairo-compile
2. add there the following:

impl From<crate::InliningStrategy> for cairo_lang_lowering::InliningStrategy {
...
}

good suggestion.

---

crates/cairo-lang-lowering/src/utils.rs line 19 at r3 (raw file):

    /// Inline only in the case of a `inline(always)` annotation
    Avoid,
    /// Never inline

as discussed - this would cause the compiler to crash - so remove this as an option.

you can (in another PR) remove all inline(always) from the corelib (aside from 2 probably, and switch them with a simple inline.

[javra]

Reviewable status: 5 of 12 files reviewed, 4 unresolved discussions (waiting on @mkaput and @orizi)

---

crates/cairo-lang-lowering/src/utils.rs line 14 at r2 (raw file):

Previously, orizi wrote…

good suggestion.

Ah yes, that's a good solution 👍

---

crates/cairo-lang-lowering/src/utils.rs line 19 at r3 (raw file):

Previously, orizi wrote…

as discussed - this would cause the compiler to crash - so remove this as an option.

you can (in another PR) remove all inline(always) from the corelib (aside from 2 probably, and switch them with a simple inline.

Thanks, can you point me towards which ones are the 2 I won't be able to remove?

---

crates/cairo-lang-lowering/src/inline/mod.rs line 68 at r1 (raw file):

Previously, orizi wrote…

Move the query out of the match.
Ok(if db.optconfig().disable..() { matches!(config, Always(_)) } else .....

Done.

[javra]

Reviewable status: 3 of 12 files reviewed, 3 unresolved discussions (waiting on @mkaput and @orizi)

---

crates/cairo-lang-lowering/Cargo.toml line 19 at r3 (raw file):

Previously, orizi wrote…

this shouldn't be a dependency of the lowering.

Done.

[orizi]

Reviewed 4 of 12 files at r2, 4 of 5 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @javra)

---

crates/cairo-lang-lowering/src/utils.rs line 19 at r3 (raw file):

Previously, javra (Jakob von Raumer) wrote…

Thanks, can you point me towards which ones are the 2 I won't be able to remove?

IIRC - basically only Option::unwrap - but not for this PR in any case.

[javra]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

---

crates/cairo-lang-lowering/src/utils.rs line 19 at r3 (raw file):

Previously, orizi wrote…

IIRC - basically only Option::unwrap - but not for this PR in any case.

Done.

[piotmag769]

Just making sure - you don't need this flag for tests compilation, do you?

Reviewed 7 of 12 files at r2, 4 of 5 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

[orizi]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @javra)

[javra]

@piotmag769 Correct

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @javra)

[piotmag769]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @mkaput)

[mkaput]

Reviewed 7 of 12 files at r2, 4 of 5 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @javra)

---

crates/bin/cairo-compile/src/main.rs line 18 at r5 (raw file):

    /// Inline only in the case of a `inline(always)` annotation
    Avoid,
}

nitpick

Suggestion:

/// Options for the `inlining` arguments.
#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, clap::ValueEnum)]
pub enum InliningStrategy {
    /// Do not override inlining strategy.
    #[default]
    Default,
    /// Inline only in the case of a `inline(always)` annotation.
    Avoid,
}

---

crates/bin/cairo-compile/src/main.rs line 44 at r5 (raw file):

    #[arg(short, long, default_value_t = false)]
    replace_ids: bool,
    /// Overrides inlining behavior

Suggestion:

    /// Overrides inlining behavior.

[javra]

Reviewable status: 10 of 12 files reviewed, 2 unresolved discussions (waiting on @mkaput, @orizi, and @piotmag769)

---

crates/bin/cairo-compile/src/main.rs line 18 at r5 (raw file):

Previously, mkaput (Marek Kaput) wrote…

nitpick

Done.

---

crates/bin/cairo-compile/src/main.rs line 44 at r5 (raw file):

    #[arg(short, long, default_value_t = false)]
    replace_ids: bool,
    /// Overrides inlining behavior

Done.

[mkaput]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @javra)

[javra]

Reviewed 7 of 12 files at r2, 2 of 5 files at r4, 2 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @javra)

[mkaput]

Reviewed 1 of 1 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @javra)

[orizi]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @javra)

[mkaput]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @javra)