[PATCH] D76096: [clang] allow const structs to be constant expressions in initializer lists

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1013
+      if (V->hasInit())
+        return Visit(V->getInit(), V->getType());
+    return nullptr;
----------------
rsmith wrote:
> efriedma wrote:
> > rsmith wrote:
> > > nickdesaulniers wrote:
> > > > efriedma wrote:
> > > > > You need to be more careful here; we can call ConstExprEmitter for arbitrary expressions.
> > > > "Be more careful" how?
> > > 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.
> > 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.
> 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?
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".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76096/new/

https://reviews.llvm.org/D76096