[CIR][CIRGen] Implement "if constexpr" code generation #436
Conversation
|
I have just realized that even |
|
@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 |
The original 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? |
keryell
force-pushed
the
if-constexpr
branch
2 times, most recently
from
fe8396d
to
712d320
Compare
clang/lib/CIR/CodeGen/CIRGenStmt.cpp
Outdated
| // Replace the "if constexpr" by its non-discarded branch | ||
| return buildStmt(ndc.value(), /*useCurrentScope=*/true); | ||
| } | ||
|
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
clang/lib/CIR/CodeGen/CIRGenStmt.cpp
Outdated
| // in this lambda like in Clang but postponed to other MLIR | ||
| // passes. | ||
| if (S.isConstexpr()) | ||
| if (auto ndc = S.getNondiscardedCase(getContext())) { |
There was a problem hiding this comment.
What should happen if the stmt is a constexpr stmt and getNondiscardedCase returns an empty/invalid result?
There was a problem hiding this comment.
Bad things happen. :-( Good catch!
There was a problem hiding this comment.
I misunderstood this API. Even if a std::optional has a value, it can be a nullptr instead of no value. Confusing.
Thanks for the detailed background.
Nope, as long as you stay within the boundaries of what's allowed to be used within LLVM (c 17 I believe?) |
No description provided.