Page MenuHomePhabricator

[clang] Implement if consteval (P1938)
ClosedPublic

Authored by cor3ntin on Sat, Sep 25, 9:20 AM.

Details

Summary

Modify the IfStmt node to tuppoort constant evaluated expressions.

Add a new ExpressionEvaluationContext::ImmediateFunctionContext to
keep track of immediate function contexts.

This proved easier/better/probably more efficient than
walking the AST backward.
It allows diagnosing nested if consteval statements.

Diff Detail

Unit TestsFailed

TimeTest
1,310 msx64 debian > libomp.api::omp_get_wtime.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -I /var/lib/buildkite-agent/builds/llvm-project/openmp/runtime/test -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -I /var/lib/buildkite-agent/builds/llvm-project/openmp/runtime/test/ompt /var/lib/buildkite-agent/builds/llvm-project/openmp/runtime/test/api/omp_get_wtime.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/test/api/Output/omp_get_wtime.c.tmp -lm -latomic && /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/test/api/Output/omp_get_wtime.c.tmp

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

So I have some heartburn about IsNegatedConsteval. A part of me wants to suggest ditching the ConstexprKind and creating a new scoped-enum IfStmtKind (perhaps in the same file) that has just None,Constexpr,Consteval,NotConsteval. Its already a bit hinky that we have a value of ConstexprKind that we don't use, so I'm leaning toward it. Other reviewers: WDYT?

Otherwise, I think clang-format REALLY needs a run here, and the codegen test can be fleshed out a bit more. Otherwise, this seems right to me.

clang/include/clang/AST/Stmt.h
1967

Not a fan of making this 'IfConstevalIsNegated' a default here... it means it has to be at the end, where it doesn't particularly make sense. Plus, you have to change all the calls to this function ANYWAY, right?

clang/lib/AST/JSONNodeDumper.cpp
1493

Is the first condition necessary here? I would think isNegatedConsteval should NEVER be true if isConsteval?

clang/lib/AST/StmtPrinter.cpp
242

You had a choice here.... You COULD have blessed the 'c++ operator names', but chose not to. I guess we all have to choose teams:D

clang/test/CodeGenCXX/cxx2b-consteval-if.cpp
2

I tend to like the 'CHECK' lines being inline with the code, it makes it easier to follow in these cases. Additionally, I think the check-lines should be more specific (that they are actually checking for 'call' instructions).

cor3ntin added inline comments.Tue, Sep 28, 8:33 AM
clang/lib/Parse/ParseStmt.cpp
1547

Yep, I realized I need to change.
Oh well.

clang/test/CodeGenCXX/cxx2b-consteval-if.cpp
2

To be honest, I adapted the code from the equivalent test for if constexpr, and I don't actually know how it work

cor3ntin updated this revision to Diff 375615.Tue, Sep 28, 9:37 AM
  • Use a dedicated enums
  • Add more tests
  • Improve CodeGen tests

A handful of 'nits' (at best), and a wish for the if consteval vs consteval if to be consistent, but otherwise LGTM. Hopefully @aaron.ballman can come and do a final pass.

clang/include/clang/AST/Stmt.h
164–165
2127–2130

Not sure if consteval if follows the same naming convention as constexpr if?

2141

Same change here.

clang/include/clang/Basic/DiagnosticSemaKinds.td
1508

This seems like it needs a newline somewhere/clang-formatting.

1508
1509
5949

I see an inconsistency in a couple of places here between if consteval or consteval if. I THINK it is the latter, so I'm suggesting changes for that. I see the DiagnosticParseKinds seem to use the consteval if spelling, so I feel like perhaps I'm not wrong?

clang/include/clang/Basic/Specifiers.h
32

I'd prefer just reverting the changes to ConstexprSpecKind to the 'before', since you're not using it anymore.

40
45

Reads nicer to me maybe? IDK, feel free to ignore.

clang/include/clang/Sema/Sema.h
1219
aaron.ballman added inline comments.Wed, Sep 29, 6:30 AM
clang/include/clang/AST/Stmt.h
164–165
167

Not sure how others feel here, but for me, I kinda hate that we're using 'unsigned' bitfields for all of this, particularly because these two are mutually exclusive.

Unfortunately I have stronger feelings here. We need the bit-fields to all have the same type, otherwise there's problems with layout in MSVC (also, we often get odd diagnostic behavior we have to come back and fix later, as in https://reviews.llvm.org/rGaf3a4e97d8627a32606ed32001583fe08f15b928). That's why we use unsigned for all our bit-fields.

2081–2105

How do folks feel about this? My concern is that if ! consteval (whatever) is still a consteval if statement, and we care "is this a consteval if statement" more often than we care which form it is, so it's more helpful that isConsteval() returns true for both forms. For the times when we care about negated vs nonnegated, we can use the more explicit functions (which have better parity with their names).

2127–2130
clang/include/clang/Basic/DiagnosticSemaKinds.td
1509

I think the diagnostic group should be named redundant-consteval-if.

clang/include/clang/Basic/Specifiers.h
45

Alternatively, ConstevalNonnegated and ConstevalNegated to make it more clear that they're distinguishable from one another. (I don't feel strongly about any particular opinion here.)

clang/include/clang/Sema/Sema.h
1219

I like the suggestion from @erichkeane, but drop the single quotes as it's no longer talking about syntax at that point.

9138

I'd rewrite to avoid the iterators entirely:

for (const ExpressionEvaluationContextRecord &Rec : llvm::reverse(ExprEvalContexts)) {
  ...
}
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
195

This probably signals that we're missing some test coverage, but this is the first I've heard of this file so I'm not clear on *how* to test it!

clang/lib/AST/JSONNodeDumper.cpp
1493

All three of these are mutually exclusive, so I think it might be better to have: kind = Ordinary | constexpr | consteval | negated consteval as an enumeration rather than having three different fields. WDYT?

clang/lib/AST/StmtPrinter.cpp
246–249
clang/lib/CodeGen/CGStmt.cpp
716–717
clang/lib/Parse/ParseStmt.cpp
1341

This is not the grammar that was adopted and should be updated accordingly. Also, please clearly mark it as C++2b grammar.

1450

This looks incorrect for a consteval if -- that requires parsing a compound statement explicitly, not just any statement.

1521–1526

Oh. I see where the diagnostic is happening. I think it's unclean to parse a statement and then diagnose it later as being the wrong kind of statement -- why not parse the correct kind of statement initially?

clang/lib/Sema/SemaStmt.cpp
930–931
937

Conversions just work this way (http://eel.is/c++draft/conv.integral#2).

clang/lib/Serialization/ASTReaderStmt.cpp
2756–2759

Can you explain these changes? I'm not certain why they were needed.

clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p4.cpp
159

Please add the newline.

erichkeane added inline comments.Wed, Sep 29, 6:39 AM
clang/include/clang/AST/Stmt.h
167

Ah shucks! Thanks for that Aaron! I still like having IfStatementKind for the interface, but it seems like we'll have to static-cast in/out here.

2081–2105

I DO think I like that, the isConstevalorNegatedConsteval was a bit of heartburn running through this, but I couldn't come up with a better spelling. I think this _IS_ that better spelling.

clang/include/clang/Basic/Specifiers.h
45

Mixed with your other suggestion on changing the functions, I think this ends up being a good suggestion.

cor3ntin updated this revision to Diff 375927.Wed, Sep 29, 9:34 AM
cor3ntin marked 20 inline comments as done.
  • Add Bytecode gen tests
  • use consteval if consistently
  • use isConsteval/isNegatedConsteval/istNonNegatedConsteval
  • Address other comments
cor3ntin added inline comments.Wed, Sep 29, 9:36 AM
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
195

Ouch, nasty! I added tests

clang/lib/AST/StmtPrinter.cpp
242

I could have :)
Also, it's easier to see that the ! appertain to consteval

clang/lib/Parse/ParseStmt.cpp
1450

See a little bit below

1521–1526

I think the code reads better that way - we need to parse any kind of statement in other cases. In any case the work is about the same

clang/lib/Sema/SemaStmt.cpp
937

It just seemed more explicit

clang/lib/Serialization/ASTReaderStmt.cpp
2756–2759

I could revert them back actually - I'm just storing the constexpr info after, which seems simpler

cor3ntin updated this revision to Diff 375970.Wed, Sep 29, 11:33 AM

use llvm::reverse

aaron.ballman added inline comments.Wed, Sep 29, 11:37 AM
clang/include/clang/AST/Stmt.h
164–165

Looks like this one was missed -- also, this needs to be unsigned instead of using the enum directly (see discussion below).

clang/include/clang/Basic/Specifiers.h
45

Sorry, Erich and I probably made this less clear -- can you go with:

enum class IfStatementKind : unsigned {
  Ordinary,
  Constexpr,
  NonnegatedConsteval,
  NegatedConsteval
};

(or flip the word order to ConstevalNonnegated if you prefer). This keeps it aligned with the names of the functions on IfStmt.

clang/include/clang/Sema/Sema.h
9138

Did this one get missed?

clang/lib/AST/JSONNodeDumper.cpp
1493

After discussing with @cor3ntin off-list, I'm retracting this suggestion -- the OnlyIfTrue bit means this will be mutually exclusive output already.

clang/lib/Parse/ParseStmt.cpp
1341–1342
1521–1526

Hmmm, this makes me a bit uncomfortable, but I'm not convinced it's a problem either. Calling ParseCompoundStmt() would require checking the token first to avoid an assert, so I can understand better wanting to avoid the messier code that comes from that. However, calling the correct parsing method also means that we don't attempt to parse something we shouldn't parse only to reject it much later; I'm a bit concerned that we'll get some bad error recovery when parsing the more general thing. However, my attempts to come up with test cases where this matters are so far falling pretty flat. One test that would help (and should pass still) is:

if consteval ({1;}); // Do we correctly reject the statement expression?
clang/lib/Sema/SemaStmt.cpp
932–934
936–938
clang/lib/Serialization/ASTReaderStmt.cpp
2756–2759

Oh, I see what's going on now. Thanks!

cor3ntin updated this revision to Diff 375974.Wed, Sep 29, 11:37 AM
cor3ntin marked 15 inline comments as done.

Fix comment

cor3ntin marked 12 inline comments as done.Wed, Sep 29, 11:40 AM
cor3ntin updated this revision to Diff 376004.Wed, Sep 29, 12:51 PM
cor3ntin marked 3 inline comments as done.

Adress Aaron's comments

aaron.ballman added inline comments.Wed, Sep 29, 12:53 PM
clang/lib/Parse/ParseStmt.cpp
1521–1526

Please hold off on changes here for the moment, as I think I may have identified a DR. Consider code like:

if consteval [[]] {
}

According to the C++ grammar, this construct should not be accepted. However, that's inconsistent with every other form of if statement and seems like a defect to me. I've asked on the Core reflectors to see.

cor3ntin added inline comments.Wed, Sep 29, 12:55 PM
clang/lib/Parse/ParseStmt.cpp
1521–1526

As discussed offline I'm letting this as is so that we can parse attributes

aaron.ballman added inline comments.Thu, Sep 30, 7:25 AM
clang/lib/Parse/ParseStmt.cpp
1521–1526

The sentiment I'm getting from the Core reflectors is that this is an oversight and there's no reason we shouldn't support attributes here.

So I think the current form is good, but I'd like some parsing test coverage for attributes:

// Test in test/Parser/
if consteval [[]] {
} else [[]] {
}

if !consteval [[]] {
} else [[]] {
}

and some AST test coverage that shows we really do attach the attributes to the substatement as expected:

// Test in test/AST/
if consteval [[likely]] {
}
cor3ntin updated this revision to Diff 376245.Thu, Sep 30, 9:23 AM

Add support for attributes

aaron.ballman accepted this revision.Mon, Oct 4, 11:25 AM
aaron.ballman added a reviewer: rsmith.

Aside from a nit, I think this is ready to go. Adding Richard in case he has the chance to give this a once-over before it lands.

clang/lib/Parse/ParseStmt.cpp
1522–1526
clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p4.cpp
5

It looks like you dropped the test for GNU statement expressions; can you add that one back (it's helpful to ensure we don't accidentally regress behavior in the future given how closely these relate to compound statements already).

This revision is now accepted and ready to land.Mon, Oct 4, 11:25 AM
cor3ntin updated this revision to Diff 376961.Mon, Oct 4, 11:32 AM

Address nits

cor3ntin updated this revision to Diff 376967.Mon, Oct 4, 11:40 AM

Fix compilation

aaron.ballman closed this revision.Tue, Oct 5, 5:05 AM

I've commit on your behalf in 424733c12aacc227a28114deba72061153f8dff2, thank you for the new functionality!