Skip to content

[clang][bytecode] Fix 'if consteval' in non-constant contexts #104707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 22, 2024
Merged

[clang][bytecode] Fix 'if consteval' in non-constant contexts #104707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions clang/lib/AST/ByteCode/Compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4355,11 +4355,6 @@ bool Compiler<Emitter>::visitReturnStmt(const ReturnStmt *RS) {
}

template <class Emitter> bool Compiler<Emitter>::visitIfStmt(const IfStmt *IS) {
if (IS->isNonNegatedConsteval())
return visitStmt(IS->getThen());
if (IS->isNegatedConsteval())
return IS->getElse() ? visitStmt(IS->getElse()) : true;

if (auto *CondInit = IS->getInit())
if (!visitStmt(CondInit))
return false;
Expand All @@ -4368,8 +4363,19 @@ template <class Emitter> bool Compiler<Emitter>::visitIfStmt(const IfStmt *IS) {
if (!visitDeclStmt(CondDecl))
return false;

if (!this->visitBool(IS->getCond()))
return false;
// Compile condition.
if (IS->isNonNegatedConsteval()) {
if (!this->emitIsConstantContext(IS))
return false;
} else if (IS->isNegatedConsteval()) {
if (!this->emitIsConstantContext(IS))
return false;
if (!this->emitInv(IS))
return false;
} else {
if (!this->visitBool(IS->getCond()))
return false;
}
Comment on lines +4366 to +4378
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really should not evaluate the condition, just the underlying statement directly
(getThen or getElse depending on both S.inConstantContext() and whether it's negated)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition is only evaluated if the ifstmt is not a consteval stmt at all

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, we should not have a emitIsConstantContext(IS) - just emit the underlying statement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what the previous code did, though. That doesn't work if we compile to bytecode and later interpret the same bytecode in a non-constant context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you deal with code like

constexpr void f() {
    if !consteval {
goto a;
a:;  
}
}```

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if that has truly not been implemented yet. Otherwise, we need to always diagnose it anyway, in the same way we do for regular if statements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole point of if consteval is to allow a runtime evaluation to do thing that would not be valid in constant context, such as asm or simd or reinterpret cast or crazy vendor extensions.

so

constexpr void f() {
if !consteval {
  // copy a volatile vla with simd and execute a gpu kernel
}
}

is intended to always work.
I don't think it would be reasonable to delay adoption of the new interpreter until we support all the features of clang in the bytecode.
So this approach is probably not viable.
If you really want to create bytecode for both branches, you need to have an escape hatch so that if the non-consteval branch cannot produce bytecode, we still can support the consteval branch without error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stuff like that needs to be "implemented" in the interpreter. It must be, because it must be rejected as well. It's the same example as the goto statement above. It needs to be compiled to bytecode, somehow. If it isn't then using it in a constexpr function (without the if consteval will also fail).

Again, this is no different than any other if statement. I could imagine compiling two different versions of functions (one for consteval = true, one for =false), but we have the same problem with other if statements. This is just a difference between the interpreting and compiling approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it took me a while, but iI think i got it... sorry for the brain fart.
Because the interpreter compile functions before they are evaluated (at least morally), we don't know in which context they are going to be executed.

This is somewhat confusing because that means which assume we can constant evaluate something that is not a constant expression, which i guess is true for constant folding.

In that light the patch makes sense, I think

I still wants to make sure that we can recover nicely in the presence of things we can't evaluate, which seems to be the case https://godbolt.org/z/bofa9b8de

Do you do the same thing for is_constant_evaluated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you do the same thing for is_constant_evaluated?

Yes:

llvm-project/clang/lib/AST/ByteCode/InterpBuiltin.cpp

Line 162 in 933f722

S.Stk.push<Boolean>(Boolean::from(S.inConstantContext()));


if (const Stmt *Else = IS->getElse()) {
LabelTy LabelElse = this->getLabel();
Expand Down
5 changes: 5 additions & 0 deletions clang/lib/AST/ByteCode/Interp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3048,6 +3048,11 @@ static inline bool Free(InterpState &S, CodePtr OpPC, bool DeleteIsArrayForm) {
BlockDesc, Source);
}

static inline bool IsConstantContext(InterpState &S, CodePtr OpPC) {
S.Stk.push<Boolean>(Boolean::from(S.inConstantContext()));
return true;
}

inline bool CheckLiteralType(InterpState &S, CodePtr OpPC, const Type *T) {
assert(T);
assert(!S.getLangOpts().CPlusPlus23);
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/AST/ByteCode/Opcodes.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,3 +780,5 @@ def AllocCN : Opcode {
def Free : Opcode {
let Args = [ArgBool];
}

def IsConstantContext: Opcode;
1 change: 1 addition & 0 deletions clang/test/CodeGenCXX/cxx2b-consteval-if.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -std=c++23 %s -emit-llvm -o - | FileCheck %s
// RUN: %clang_cc1 -std=c++23 %s -emit-llvm -o - -fexperimental-new-constant-interpreter | FileCheck %s

void should_be_used_1();
void should_be_used_2();
Expand Down
Loading