This is an archive of the discontinued LLVM Phabricator instance.

[clang] Implement if consteval (P1938)
ClosedPublic

Authored by cor3ntin on Sep 25 2021, 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

Event Timeline

cor3ntin created this revision.Sep 25 2021, 9:20 AM
cor3ntin requested review of this revision.Sep 25 2021, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2021, 9:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
cor3ntin updated this revision to Diff 375045.Sep 25 2021, 9:40 AM

Remove untested clang-format changes

cor3ntin updated this revision to Diff 375046.Sep 25 2021, 9:58 AM

Fix tests formatting

cor3ntin updated this revision to Diff 375065.Sep 25 2021, 12:33 PM

Update cxx_status.html

erichkeane added inline comments.Sep 27 2021, 8:57 AM
clang/include/clang/AST/Stmt.h
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. I'd prefer (though listen to others here first) if the type of this was something more like:

IfEvalKind EvalKind : 2; // where IfEvalKind is enum class IfEvalKind {None, Constexpr, Consteval};

clang/include/clang/Basic/DiagnosticSemaKinds.td
5942

Oh boy.... this section of 'notes' seem like they deserve some level of merging with a %select. I'd not want anything other than the jump enters controlled statemennt of variants for this patch though.

clang/include/clang/Sema/Sema.h
1320

I get this NOW that I'm looking at it, but isImmediate made me go to assembly-immediate, though perhaps this is only misleading to me. I might bikeshed this to isImmediatelyEvaluated or something.
Anyone else, WDYT?

4723–4724

can this be a scoped enum?

Also, perhaps pulled into the AST in a way that it can be reused for the IfStmt state.

9132

What is our iterator type? I THINK at minimum require this to be auto *

clang/lib/AST/Stmt.cpp
926

Oh dear... I'm not quite sure I want this as a scoped enum.

clang/lib/AST/StmtPrinter.cpp
246

should this be If->getElse?

clang/lib/Parse/ParseStmt.cpp
1363

Is this diag here right? We're in the kw_consteval branch, but it seems we're warning about constexpr-if?

1375

Why is this here instead of with the kw_consteval handling?

clang/lib/Sema/SemaStmt.cpp
879–881

perhaps an assert here: assert((CondExpr || Kind == IfKind_Consteval) && "....")

cor3ntin updated this revision to Diff 375306.Sep 27 2021, 9:41 AM
cor3ntin marked 3 inline comments as done.
  • Rename isImmediate to isImmediateFunctionContext
  • Fix And cleanup Diagnostics in ParseStmt.cpp
  • Add assert in SemaStmt.cpp
  • Fix StmtPrinter.cpp
cor3ntin added inline comments.Sep 27 2021, 9:43 AM
clang/include/clang/AST/Stmt.h
167

I was not quite sure where to put an enum I could reuse in different place.
But I think I'd agree with you otherwise.
Any suggestion for where to put it?

clang/include/clang/Sema/Sema.h
9132

The iterator type is ungodly. Likely SmallVector<ExpressionEvaluationContextRecord, 8>::reverse_iterator. I think auto here is more readable but I can change if you want to

clang/lib/Parse/ParseStmt.cpp
1363

Yup, forgot to remove that

1375

To not have a warning when there is an error. Maybe we don't care?
In any case, I've cleanup that

Note I didn't have a good chance to re-review this, but wanted to give my feedback to the comments right away.

clang/include/clang/AST/Stmt.h
167

My best suggestion is somewhere in include/Basic.

We don't have a great file I think to fit it, but we DO have a ton where we've created files for smal lthings (see ExceptionSpecificationType.h, Linkage.h, or Lambda.h).

Looking through, there _IS_ a similar enum in Specifiers.h (perhaps the best place for this) that might work well, ConstexprSpecKind might actually just be what we want and could replace the other enum you created later.

clang/include/clang/Sema/Sema.h
9132

I more questioned due to the lack of the *, I see it is the reverse-iterator now, so i don't think that makes it a pointer type, so I don't think it is valuable here. @aaron.ballman is our 'auto nitpicker', so I'll let him to suggest the right answer.

clang/lib/Parse/ParseStmt.cpp
1377

So I still meant here putting it inside the 'if (Tok.is(tok::kw_consteval)' bit, I have a slight preference to match the rest of these sorts of warnings.

cor3ntin updated this revision to Diff 375352.Sep 27 2021, 11:54 AM

use ConstexprSpecKind to describe whether the IfStmt
is constexpr or consteval.
Other minor fixes

erichkeane added inline comments.Sep 27 2021, 12:17 PM
clang/lib/AST/Stmt.cpp
917

Does this need clang-format again?

926

I see I mistyped here, i'm 'NOW' quite sure. Thanks for fixing it anyway.

clang/lib/AST/StmtPrinter.cpp
239

I think I'm not a fan of the inversion mostly for this, the fact that we end up printing the reverse of what happens in 'if constexpr' is giving me some heartburn.

clang/lib/Analysis/CFG.cpp
3050

I think there is a ternary-confuse-warning in GCC that fires sometimes if you skip outer-parens, you might find it necessary/a good idea to put parens around the condition.

3064–3066

Note for future-reviewer: This ends up being an improvement here, TryResult contains only a single integer in a VERRY bizarre way, it seems to be intended to essentially be a 'true/false/unknown' bit of hackery (despite having 3 states, its an integer?). If someone runs across this comment in the future and wants an 'easy win', I suspect swapping it with a llvm::Optional<bool> or something would be at least an interface-win.

clang/lib/Parse/ParseStmt.cpp
1541

So this is interesting. I'm not sure how I feel about having the AST not represent the textual representation like this. I'm interested what others have to say.

My understanding is that this makes:

if consteval { thenstmt; } else { elsestmt;
be represented as:
IfStmt isConsteval, with getThen()== thenstmt

however
if not consteval { thenstmt; } else { elsestmt;}
be represented as:
IfStmt isConsteval, with getThen()== elsestmt

IMO, this feels too clever.

I think I'd prefer that the IfStmt know whether it is a 'not consteval' and select the right one that way.

aaron.ballman added inline comments.Sep 27 2021, 12:24 PM
clang/lib/Parse/ParseStmt.cpp
1541

I haven't had the chance to go over this review yet, but this comment caught my eye in my inbox so I figured I'd respond quickly.

The current approach is definitely clever, but I don't think it's the right way to tackle this. Generally, the AST needs to retain enough fidelity to be pretty printed back out to the original source, which wouldn't work here. But also, this makes it harder to write AST matchers over the construct because it's not really matching what the user wrote in source (we sometimes get around this by having a "semantic" and "syntactic" AST representation, but that seems like overkill here).

cor3ntin added inline comments.Sep 27 2021, 12:24 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
5942

I don't think we can change that without some more refactor.
see usages of note_protected_by_constexpr_if.
The Diag ID is passed to a GotoScope emitting later. No way to pass parameter. Changing that would be unrelated to this PR

erichkeane added inline comments.Sep 27 2021, 12:26 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
5942

good enough for me for now.

cor3ntin added inline comments.Sep 27 2021, 12:29 PM
clang/lib/Parse/ParseStmt.cpp
1541

This is exactly the wording though.

An if statement of the form if ! consteval compound-statement is not itself a consteval if statement, but is equivalent to the consteval if statement if consteval { } else compound-statement

An if statement of the form `if ! consteval compound-statement1 else statement2` is not itself a consteval if statement, but is equivalent to the consteval if statement

Doing something else would require storing the not position, and I don't think we gain any functionality?

aaron.ballman added inline comments.Sep 27 2021, 12:36 PM
clang/lib/Parse/ParseStmt.cpp
1541

Sure, the standard also talks about how a for loop is equivalent to a while statement, but we still don't model our AST that way. We gain functionality with the pretty printing facilities actually working and being able to AST match more naturally (like wanting to match a not predicate of an if constexpr statement).

cor3ntin updated this revision to Diff 375370.Sep 27 2021, 12:41 PM

Formatting

erichkeane added inline comments.Sep 27 2021, 12:43 PM
clang/lib/Parse/ParseStmt.cpp
1541

I don't think you'd necessarily have to store the 'not' position, just the 'not-ness'

cor3ntin updated this revision to Diff 375374.Sep 27 2021, 12:45 PM
cor3ntin marked 3 inline comments as done.

Add parens in CFG.cpp

cor3ntin updated this revision to Diff 375487.Sep 28 2021, 1:22 AM
  • Add IfStmt::isNegatedConsteval to track whether the if

consteval statement was negated

  • Add json/text AST dumps tests
martong removed a subscriber: martong.Sep 28 2021, 1:43 AM

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.Sep 28 2021, 8:33 AM
clang/lib/Parse/ParseStmt.cpp
1541

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.Sep 28 2021, 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
2122

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

2131

Same change here.

clang/include/clang/Basic/DiagnosticSemaKinds.td
1505

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

1505
1506
5945

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
1223
aaron.ballman added inline comments.Sep 29 2021, 6:30 AM
clang/include/clang/AST/Stmt.h
164
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–2095

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).

2122
clang/include/clang/Basic/DiagnosticSemaKinds.td
1506

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
1223

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

9132

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
715–716
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.

1449

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

1520–1525

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
922–923
929

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

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

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

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

Please add the newline.

erichkeane added inline comments.Sep 29 2021, 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–2095

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.Sep 29 2021, 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.Sep 29 2021, 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
1449

See a little bit below

1520–1525

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
929

It just seemed more explicit

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

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

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

use llvm::reverse

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

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
9132

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
1520–1525

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
924–926
928–930
clang/lib/Serialization/ASTReaderStmt.cpp
2756–2758

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

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

Fix comment

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

Adress Aaron's comments

aaron.ballman added inline comments.Sep 29 2021, 12:53 PM
clang/lib/Parse/ParseStmt.cpp
1520–1525

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.Sep 29 2021, 12:55 PM
clang/lib/Parse/ParseStmt.cpp
1520–1525

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

aaron.ballman added inline comments.Sep 30 2021, 7:25 AM
clang/lib/Parse/ParseStmt.cpp
1520–1525

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.Sep 30 2021, 9:23 AM

Add support for attributes

aaron.ballman accepted this revision.Oct 4 2021, 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
1521–1525
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.Oct 4 2021, 11:25 AM
cor3ntin updated this revision to Diff 376961.Oct 4 2021, 11:32 AM

Address nits

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

Fix compilation

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

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