constcombine pass optimizing unary and binary operators

[xunilrj]

Description

Closes #4708.

This PR does NOT change the IR generation for expressions. Instead, it implements constant folding for unary and binary operators. The idea is that IR generation will be kept simple and stable, and all optimizations are pushed onto the opt passes.

To better test this, this PR also changes some implementation details on IR generation tests:

1 - filecheck directives are now collected and tested in groups. This allows a new "::check-ir-optimized::", that can be configured with the passes you want to test, or just fallback to "o1".

2 - filechecker explanations are now pretty-printed, and can be printed when verbose=true. This allows the test to be checked more easily.

breaking label because of this change: https://github.com/FuelLabs/sway/pull/4780/files#diff-b40fc3999876b0ff447de112b508250c7d0e7993f9023db29ddce8590d9b2286L5

Checklist

• I have linked to any relevant issues.
• I have commented my code, particularly in hard-to-understand areas.
• I have updated the documentation where relevant (API docs, the reference, and the Sway book).
• I have added tests that prove my fix is effective or that my feature works.
• I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
• I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
• I have requested a review from the relevant team or maintainers.

[vaivaswatha]

Instead, it implements constant folding for unary and binary operators. The idea is that IR generation will be kept simple and stable, and all optimizations are pushed onto the opt passes.

So this optimizes patterns of the kind in #4708.

fn main() -> bool {
    let result: u64 = 0 + 1 + 2 + 3 + 4 + 5;

But what if we have const foo = 0 + 1 + 2 + 3 + 4 + 5 declared globally? That wouldn't be const-evaluated right?

While I'm not entirely against this approach, aren't we duplicating const-evaluation logic?

[xunilrj]

But what if we have const foo = 0 + 1 + 2 + 3 + 4 + 5 declared globally? That wouldn't be const-evaluated right?

It would be const-eval-ed, because it is a "constant declaration".

While I'm not entirely against this approach, aren't we duplicating const-evaluation logic?

Partially. The intersection today is quite small actually.

The question can also be: "can we completely replace const-eval by the optimizer?" Although this may be possible, there is the chance that the expression do not evaluate to const and the optimizer would need to fail the whole compilation. Besides, we would need to run the optimizer even on debug builds.

So that is why I do see them as different.

My rationale in this case was:

1 - const declarations must evaluate expressions to constants. So the const_eval module folds as much as possible to constants and it guarantees that it is safe to do so. Failing the compilation when this is not possible.

2 - If we try to solve "constant folding" before generating the IR, we just need to const_eval every expression and check if they fold to a constant or not. This will tremendously increase compilation time.

We can alleviate this by limiting const_eval to initializations only. We will still have a little bit of performance impact, and the issue of constant folding not propagating to other obvious expressions like function calling or not inlining calls etc...

This means that the "expected behaviour", constant folding mixing well with other optimizations, will only be achieved with a fully complete "constant folding" inside the optimizer.

3 - Thus, makes more sense to do no optimization on IR generation. Having the benefits of faster compilation and a more stable IR.

[vaivaswatha]

If we try to solve "constant folding" before generating the IR, we just need to const_eval every expression and check if they fold to a constant or not. This will tremendously increase compilation time.

Likely not. Most const-folding will fail early, I wouldn't expect this to significantly impact compile time at all. The advantage being that we'll have all const-folding done in one place.

@IGI-111 @anton-trunov any thoughts?

[IGI-111]

I've been wondering if we're going the wrong way and doing too much too early actually. Maybe we should just check const-eval-ness using semantic checks (and syntax markers) and let the optimizer do it's thing.

Ultimately we care more about binary size than we do about compile time but there might not be tension.

What do the benchmarks on this say?

[vaivaswatha]

we should just check const-eval-ness using semantic checks (and syntax markers) and let the optimizer do it's thing.

Checking for that pretty much is as good as doing the optimization itself. You actually need to evaluate and see that we have a valid const (for example, you can't know something might overflow until you evaluate). So this means that we need to do all of this at the early stage. Why not use the same code right there for non-const expressions too? If it can't be easily reused, then I agree that doing it later like this is the right way.

btw, I'm not strongly opposed to this, so I wouldn't mind if we merge this PR (after a proper review). More importantly, the algorithm (both new and existing ones there) are O(n^2), but easily need not be. We should file an Issue for that.

[xunilrj]

btw, I'm not strongly opposed to this, so I wouldn't mind if we merge this PR (after a proper review).

I think we can follow with the proper review of this PR, then.

More importantly, the algorithm (both new and existing ones there) are O(n^2), but easily need not be. We should file an Issue for that.

Yes. I pretty much just copied what was there. I can create a PR with proper timing to measure improvements later.

Why not use the same code right there for non-const expressions too? If it can't be easily reused, then I agree that doing it later like this is the right way.

I can create another PR after this one, using the const_eval and we can assess if we like the solution or not.

[IGI-111]

Alright, I think we can review and merge this. But I'll want to see benchmarks for further changes.

[vaivaswatha]

Is there an advantage of the newly added #cfg[test] tests? With the improvements to ir_generation tests in this PR, it appears that they subsume the in-source testing code added. So we could just move the tests to that single infrastructure rather than doing similar testing in two places.

The improvements to ir_generation tests are nice !. Thank you.

Everything else looks good to me. Once we converge on my point above, I'll approve.

[xunilrj]

You mean the tests inside sway-ir/src/optimize/constants.rs and sway-ir/src/optimize.rs?

I think they are useful there because tests get much more dense and localized.
A simple "find all references" find tests, for example.

[vaivaswatha]

You mean the tests inside sway-ir/src/optimize/constants.rs and sway-ir/src/optimize.rs?

I think they are useful there because tests get much more dense and localized. A simple "find all references" find tests, for example.

Yes, I mean those tests. Those tests (or specifically the testing code) do not cover anything that isn't covered by the ir_gemeration tests. So this testing may as well be done there. I agree about the searchability but that's coming at the cost of duplication of testing code. Maybe somehow improve searching for the existing code itself?

[xunilrj]

They are testing more cases tough.
I would need to test all edge cases, all operators etc... on the IR generation tests.

[vaivaswatha]

They are testing more cases tough. I would need to test all edge cases, all operators etc... on the IR generation tests.

Hmmm ok. btw, you can refactor the two functions to avoid duplication of some of the code.

1. Have just one function that takes an Option of expected result. If it's Some then it's supposed to succeeded.
2. or have three functions with the common code in one

[xunilrj]

Good idea. Done.