Page MenuHomePhabricator

[clang] allow const structs to be constant expressions in initializer lists
Needs ReviewPublic

Authored by nickdesaulniers on Mar 12 2020, 1:48 PM.

Details

Summary

This seems to be an undocumented GNU C extension.

For code like:
struct foo { ... };
struct bar { struct foo foo; };
const struct foo my_foo = { ... };
struct bar my_bar = { .foo = my_foo };

during CodeGen of LLVM IR, copy the initializer of my_foo into the
initializer of my_bar recursively.

Eli Friedman points out the relevant part of the C standard seems to
have some flexibility in what is considered a constant expression:

6.6 paragraph 10:
An implementation may accept other forms of constant expressions.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2020, 1:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
  • add 2 missing CHECKs
nickdesaulniers marked an inline comment as done.Mar 12 2020, 1:56 PM
nickdesaulniers added inline comments.
clang/lib/AST/Expr.cpp
3164

Interesting, playing with this more in godbolt, it looks like the struct doesn't even have to be const qualified.

nickdesaulniers marked an inline comment as not done.Mar 12 2020, 1:59 PM
nickdesaulniers added inline comments.
clang/lib/AST/Expr.cpp
3164

Or, rather, behaves differently between C and C++ mode;

C -> const required
C++ -> const not required

Harbormaster completed remote builds in B49050: Diff 250044.

I would like to kill off all the code you're modifying, and let ExprConstant be the final arbiter of whether a constant is in fact constant. But that's probably a larger project than you really want to mess with. The big missing piece of that is that we currently don't allow ExprConstant to evaluate structs/arrays in C, for the sake of compile-time performance. (Not sure what the performance impact for large arrays looks like at the moment.)

clang/lib/AST/Expr.cpp
3164

In C++, global variable initializers don't have to be constant expressions at all.

Do we really need to check GNUMode here? We try to avoid it except for cases where we would otherwise reject valid code.

Do we need to worry about arrays here?

clang/lib/CodeGen/CGExprConstant.cpp
1013

You need to be more careful here; we can call ConstExprEmitter for arbitrary expressions.

nickdesaulniers marked an inline comment as done and an inline comment as not done.Mar 12 2020, 3:55 PM

But that's probably a larger project than you really want to mess with.

Maybe with some coaching; but if this is already supported today in C++, maybe it's just a small incision to support this in C?

clang/lib/AST/Expr.cpp
3164

In C++, global variable initializers don't have to be constant expressions at all.

It looks like my test cases are supported already in Clang today, in C++ mode only and not C. Maybe there's some alternative code path that I should be looking to reuse?

Do we really need to check GNUMode here?

Maybe a -Wpedantic diag would be more appropriate otherwise? (GCC does not warn for these cases with -Wpedantic. If the answer to your question is no, then that means supporting these regardless of language mode. (I'm ok with that, was just being maybe overly cautious with GNUMode, but maybe folks with better knowledge of the language standards have better thoughts?)

Do we need to worry about arrays here?

I don't think arrays are supported: https://godbolt.org/z/RiZPpM

clang/lib/CodeGen/CGExprConstant.cpp
1013

"Be more careful" how?

efriedma added inline comments.Mar 12 2020, 4:26 PM
clang/lib/AST/Expr.cpp
3164

Also, do we need to check that we actually have a definition for the variable?

clang/lib/AST/Expr.cpp
3164

The C++ standard is substantially different from C. C++ global initializers can be evaluated at runtime. So we don't call this code at all in C++.

Independent of that, we do have pretty complete support for constant evaluation of structs in C++ to support constexpr, and we should be able to leverage that.


For arrays, I was thinking of something like this:

const int foo[3] = { 0, 1, 2 };
int bar = foo[0];

We generally don't generate pedantic warnings unless the user uses an extension that's disallowed by the C standard. (The idea is that clang with -pedantic should generate a diagnostic every place the C standard requires a diagnostic. It's not a catch-all for extensions.)

We could separately generate some sort of portability warning, but not sure anyone would care to enable it.

nickdesaulniers planned changes to this revision.Mar 13 2020, 9:42 AM

The performance implications of deleting those lines is the complicated part.

Where does compile time performance suffer from this? I guess if we have massive array initializers, or large struct definitions, or deeply nested struct definitions, it might take time to recursively evaluate if all members are constant expressions; but isn't that what I want as a developer, to offload the calculations to compile time rather than runtime? Or is the cost way too significant? Looks like @rsmith added those comments/checks back in 2012 via commit dafff947599e ("constexpr irgen: Add irgen support for APValue::Struct, APValue::Union,").

Let me see if I comment out the code I've added, then modify those two spots in ExprConstant.cpp you pointed out, if that works as well.

clang/lib/AST/Expr.cpp
3164

Do we really need to check GNUMode here?

Will remove.

Do we need to worry about arrays here?

Yep; as long as the base and index are const qualified, GCC allows them. Will add tests.

Also, do we need to check that we actually have a definition for the variable?

Yep, will add tests.

It's not a catch-all for extensions.

Ah, got it.

The performance implications of deleting those lines is the complicated part.

Where does compile time performance suffer from this? I guess if we have massive array initializers, or large struct definitions, or deeply nested struct definitions, it might take time to recursively evaluate if all members are constant expressions; but isn't that what I want as a developer, to offload the calculations to compile time rather than runtime? Or is the cost way too significant?

It's exactly what you suspect: very large global arrays initialized from constant data. For those, it's substantially more efficient for CodeGen to walk the AST representation (the InitListExpr) and directly generate an IR constant than it is to create an APValue representation of the array. (APValue is not especially space-efficient, and the extra copying and data shuffling can be quite slow.) We saw a significant compile time regression on a couple of LNT benchmarks without these early bailouts for C.

Removing those two LangOpt checks isn't enough for the test cases to run.

Removing those two checks should unblock const-eval for structs in general. I wasn't thinking about whether there something else for global variables specifically, though. It looks like there's a relevant FIXME in findCompleteObject() in ExprConstant.cpp.

APValue is not especially space-efficient

Yes. We should probably do something similar to what we do for LLVM constants: add specialized representations for simple cases, like ConstantDataArray.

  • add support for compile time arrays

it's substantially more efficient for CodeGen to walk the AST representation (the InitListExpr) and directly generate an IR constant than it is to create an APValue representation of the array. (APValue is not especially space-efficient, and the extra copying and data shuffling can be quite slow.)

Isn't that what my patch is doing? (Codegen walking the AST/InitListExpr, generating Constants)?

Uploading what I have; handling arrays is trickier than structs; the index and the base both end up having complex subtrees to fetch values from. As much fun as it is to build a compile time evaluator, it sounds like I should stop pursing CGExprConstant visitors in favor of ExprConstant? (I guess it's not clear to me whether @rsmith is in agreement with @eli.friedman on whether we want to be more aggressive in compile time evaluation or not).

Isn't that what my patch is doing? (Codegen walking the AST/InitListExpr, generating Constants)?

Yes. The issue is we we don't really want to duplicate the logic.

nickdesaulniers edited the summary of this revision. (Show Details)Mar 16 2020, 4:06 PM
rsmith added inline comments.Mar 16 2020, 4:19 PM
clang/lib/CodeGen/CGExprConstant.cpp
1013

Here are some specific cases in which you need to be more careful because the code as-is would do the wrong thing:

  • when emitting a global's initializer in C++, where the value of the object denoted by the DeclRefExpr could have changed between its initialization and the expression we're currently emitting
  • when emitting anything other than a global initializer in C, where the value of a global could have changed after its emission
  • when emitting a reference to a non-global declaration in C (local variables might change between initialization and use)

We would need to restrict this to the cases where the variable's value cannot possibly have changed between initialization and use.

In C, that's (mostly) the case for a static storage variable referenced from the initializer of a static storage variable, for a thread storage variable referenced from the initializer of a static storage variable, or for a thread storage variable referenced from the initializer of a thread storage variable. Even then, this isn't strictly correct in the presence of DSOs, but I think it should be correct if the two variables are defined in the same translation unit.

In C++, that's (mostly) the case when the variable is const or constexpr and has no mutable subobjects. (There's still the case where the reference happens outside the lifetime of the object -- for the most part we can handwave that away by saying it must be UB, but that's not true in general in the period of construction and period of destruction.)

In both cases, the optimization is (arguably) still wrong if there are any volatile subobjects.

efriedma added inline comments.Mar 16 2020, 4:50 PM
clang/lib/CodeGen/CGExprConstant.cpp
1013

And this is why I don't want to duplicate the logic. :)

I'd rather not make different assumptions for C and C++; instead, I'd prefer to just use the intersection that's safe in both. I'm concerned that we could come up with weird results for mixed C and C++ code, otherwise. Also, it's easier to define the C++ rules because we can base them on the constexpr rules in the standard.

rsmith added inline comments.Mar 16 2020, 5:15 PM
clang/lib/CodeGen/CGExprConstant.cpp
1013

I agree we probably want the same outcome as D76169 provides, if either the performance is acceptable or we can find a way to avoid the performance cost for the cases we already accept. Perhaps we could get that outcome by ensuring that we try the CGExprConstant fast-path (at least in C compilations, maybe in general) before we consider the complete-but-slow evaluation strategy in ExprConstant?

efriedma added inline comments.Mar 16 2020, 6:20 PM
clang/lib/CodeGen/CGExprConstant.cpp
1013

I like the idea of guarding constant evaluation with a "fast path" that doesn't actually compute the APValues in simple cases. We can just implement the simple stuff, and fall back to the full logic for complicated stuff.

My one concern is that we could go to a bunch of effort to emit a variable on the fast path, then fall off the fast path later because "return a[0];" tries to constant-fold a big array "a".