[CIR][CIRGen] Implement "if constexpr" code generation

[keryell]

No description provided.

[keryell]

I have just realized that even if constexpr statements can have an initialization statement too.
I need to extend this PR.

[bcardosolopes]

@keryell thanks for working on this! We usually try to follow the skeleton of traditional LLVM codegen, for example, there are no top level calls to S.getNondiscardedCase(getContext())) on EmitIfStmt. Please try to use the equivalent call that leads to that path in buildIfStmt (making any necessary adjustments and/or explaining differences in comments).

[keryell]

@keryell thanks for working on this! We usually try to follow the skeleton of traditional LLVM codegen, for example, there are no top level calls to S.getNondiscardedCase(getContext())) on EmitIfStmt. Please try to use the equivalent call that leads to that path in buildIfStmt (making any necessary adjustments and/or explaining differences in comments).

The original CodeGenFunction::EmitIfStmt is cleverly designed since the code can handle all kinds of if and has a partial evaluation in the case the condition is known at compile time. The handling of if constexpr is just done as a detail of this partial evaluation.
In CIR, the choice has been made to postpone this partial evaluation to some future CIR MLIR passes, which makes sense in the CIR spirit. But then we can no longer use the generic if partial evaluation to implement the if constexpr.
So, either this code is copy-pasted in this context and restricted to if constexpr to avoid touching normal if or we use this nice Stmt::getNondiscardedCase() with a modern monadic interface which exactly solved this issue. The original verbose code uses ConstantFoldsToSimpleInteger which is not really a low-level API either.
As a trade-off I have added an if (S.isConstexpr()) short-cut to avoid Stmt::getNondiscardedCase() digging into the non getCond()->isValueDependent() case of IfStmt::getNondiscardedCase().

As a more general question, since the main code is from 2008, from before modern C++, does this preclude using modern API for new developments?

[bcardosolopes]

The comment I left on the code is talking about early optimizations done in LLVM codegen that aren't necessarily constexpr related. I believe we can still follow the same skeleton. For instance, instead of adding this here, have you tried to implement this inside the check below:

    if (ConstantFoldsToSimpleInteger(S.getCond(), CondConstant,
                                     S.isConstexpr())) {
     // In CIR we don't flip the conditions right away
     if (S.isConstexpr()) {
        RunCleanupsScope Scope(*this);
        buildStmt(S);
      }
      ...
     }
      assert(!UnimplementedFeature::constantFoldsToSimpleInteger());
    }

If you did, what didn't work?

[bcardosolopes]

Note that expensive checker is still useful because we can track of that missing optimizations because of the assert(!UnimplementedFeature::constantFoldsToSimpleInteger());. Once we have a passa that implements this, we could use your thiner version (just leave guidance in the comments).

[keryell]

Yes, it works but the current code calling the ifStmtBuilder lambda always creates a cir.scope while Clang can avoid generating anything in that case.
Cf keryell@a547eab#diff-0cb68c7505de027f386b8b1c8a4bfdf4b2f26978d71c8a52b3447116715ef7f5R77 generated from keryell@a547eab#diff-0cb68c7505de027f386b8b1c8a4bfdf4b2f26978d71c8a52b3447116715ef7f5R31
Is this OK or do I need to rework the logic of ifStmtBuilder and its caller?

[bcardosolopes]

This is fine, the cir.scope is created to represent the overall scope where the if codegen actually happens within (it's where init statements will place their alloca, etc). Poking at the IfStmt some layers above for this optimization isn't worth it IMO - the cleanup pass should be stripping the cir.scope here anwyays, but let me track that as a separate issue.

Thanks for improving the PR, LGTM!

[bcardosolopes]

What should happen if the stmt is a constexpr stmt and getNondiscardedCase returns an empty/invalid result?

[keryell]

Bad things happen. :-( Good catch!

[keryell]

I misunderstood this API. Even if a std::optional has a value, it can be a nullptr instead of no value. Confusing.

[bcardosolopes]

@keryell thanks for working on this! We usually try to follow the skeleton of traditional LLVM codegen, for example, there are no top level calls to S.getNondiscardedCase(getContext())) on EmitIfStmt. Please try to use the equivalent call that leads to that path in buildIfStmt (making any necessary adjustments and/or explaining differences in comments).

The original CodeGenFunction::EmitIfStmt is cleverly designed since the code can handle all kinds of if and has a partial evaluation in the case the condition is known at compile time. The handling of if constexpr is just done as a detail of this partial evaluation. In CIR, the choice has been made to postpone this partial evaluation to some future CIR MLIR passes, which makes sense in the CIR spirit. But then we can no longer use the generic if partial evaluation to implement the if constexpr. So, either this code is copy-pasted in this context and restricted to if constexpr to avoid touching normal if or we use this nice Stmt::getNondiscardedCase() with a modern monadic interface which exactly solved this issue. The original verbose code uses ConstantFoldsToSimpleInteger which is not really a low-level API either. As a trade-off I have added an if (S.isConstexpr()) short-cut to avoid Stmt::getNondiscardedCase() digging into the non getCond()->isValueDependent() case of IfStmt::getNondiscardedCase().

Thanks for the detailed background.

As a more general question, since the main code is from 2008, from before modern C++, does this preclude using modern API for new developments?

Nope, as long as you stay within the boundaries of what's allowed to be used within LLVM (c++17 I believe?)