optimizing Use expressions inside if condition

[azizghuloum]

First time contributor here.

While debugging the issue, I noticed that the HIR tree for the "slow" part contained Use expressions that were not handled in any special way during lowering to MIR, and thus introduced a temporary variable and lead to the performance degradation reported.

The change of course affects how MIR is generated, and thus affects a number of tests. In particular, it makes one (ui) test pass when it used to compile with error. The other tests affected, according to my (shallow) analysis, look correct.

Feedback appreciated.

Fixes #111583

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[petrochenkov]

r? rust-lang/compiler

[cjgillot]

Could you add a reduced version of the code in #111583 as a mir-opt test? In the tests/mir-opt/pre-codegen directory there are several examples.

I'm a bit worried by this change. ExprKind::Use is responsible for dropping temporaries. The ui test changing is probably a symptom. Could you elaborate why this change is sound?

cc @dingxiangfei2009 as have better knowledge of MIR scoping than I

[azizghuloum]

Thanks for the review @cjgillot.

I think the change is sound based on how ExprKind::Use was handled in the general case in https://github.com/rust-lang/rust/blob/master/compiler/rustc_mir_build/src/build/expr/into.rs#L287 where it's basically dropped and the source is used.

As for the UI change, the code is basically:

pub fn foo(){
    let x;
    if true && {x = 4; true} && x == 4 {}
}

which currently does not compile (x is possibly uninitialized). Now consider this equivalent function:

pub fn bar(){
    let x;
    if true {
       if {x = 4; true} && x == 4 {}
    }
}

This one does compile today but if you transform it to the equivalent function foo (which is a correct transformation), it doesn't compile. I believe that either both foo and bar should compile, or both should err, but not one and not the other.

Please correct me if I'm wrong in any of that. I could be missing something.

As for the test cases, I would add them if I'm on the right track.

[dingxiangfei2009]

@azizghuloum Thank you for your PR! I would like to supply additional information for the review.

The purpose of ExprKind::Use is indeed for scoping in MIR. It is used for inserting drops while building MIRs, but it is an indirect effect because its sole purpose is a "marker" to insert instructions to drop temporaries while evaluating the source.

By bypassing the as_temp step, the temporaries are then dropped at a later program point because the effective terminating scope of them are higher than we would like it to be. Maybe our current test suite has not tested this yet. That could be a source of problem.

There are two interesting questions remained. One is how shall we also admin the program in #111644 (comment) and another is why the performance degradation happens. I would need some extra time on the investigation. I will be right back.

[azizghuloum]

Note to self. The thir::ExprKind::Use came from a hir::ExprKind::DropTemps which was introduced here: https://github.com/rust-lang/rust/blob/master/compiler/rustc_ast_lowering/src/expr.rs#L421

From reading the code, it seemed like having a "chained" let anywhere would make it NOT drop temporaries.

So, strangely, this compiles:

#![feature(let_chains)]
pub fn foo() {
    let x;
    if let _t = 0 && true && {x = 4; true} && x == 4 {}
}

while this doesn't:

pub fn foo() {
    let x;
    if true && {x = 4; true} && x == 4 {}
}

which is leading me to think that there is more that is broken than it seemed at first.

[azizghuloum]

Working on another solution which does not affect lifetimes... testing now.

[dingxiangfei2009]

@azizghuloum I digged deeper into the code today and I would like to say that I am glad that you have kept this PR open. I believe it is still safe to unwrap the ExprKind::Use here. The reason is that the scope information has been computed in the lowering to THIR, which I overlooked. From now on, as_temp and eventually as_place will consult with the temp_lifetime scoping information to insert the drops.

cc @cjgillot

Also, the reason that employing let_chains makes the example compile is because let-chains are handled completely differently. Chaining lets can only be written and connected by &&. Exploiting this feature, the match failure cases are always routed to breaking into the else block. For example, the version with the let-chain gets compiled into this:

Without let-chain and without your patch, it would look like this:

Now you would see that, the control flow would always take on a path on which _1 is initialized. However, this fact can only be exploited after running the ConstProp and SimplifyConstCondition MIR optimization passes which are applied only after the MIR borrow checking. Your patch, however, would successfully mitigate this issue in my humble opinion. 👍

[cjgillot]

@azizghuloum like I asked in #111752, could you add a test for this new behaviour in the mir-opt/built directory? There are examples in that directory.

That test should involve each term in the logical expression to have stuff to drop (struct Droppy; with an empty Drop impl should be enough), so we can see that this remains correct.

This PR makes new code compile, so we'll need a signoff from t-lang.

[azizghuloum]

@cjgillot is this the test you were looking for to ensure drops are handled properly? I'll add another test from issue #111583 .

[azizghuloum]

@cjgillot what's a "signoff from t-lang"? Sorry, new here.

[azizghuloum]

Did add the test, and as I expected, there are now two drop calls for each Droppy (one drop at top of the consequent side and one at the top of the alternate side). Dunno if this is a problem but that's why I was going for the alternative solution at #111725.

[azizghuloum]

Greetings again,

I have fixed PR #111725 to

1. perform the desired optimization but at AST->HIR lowering level (instead of HIR->MIR lowering which this one does)
2. not skip over the Use expressions (which this one does)
3. not emit duplicate drop calls (which this one does)
4. have the two new tests that I've added here.

Can we review that one and close this PR please? @cjgillot @dingxiangfei2009

[cjgillot]

Replaced by #111725