rjmccall added inline comments.
================
Comment at: lib/CodeGen/CGExprConstant.cpp:1413
+ } else if (!Init->isEvaluatable(CE.CGM.getContext())) {
+ return false;
+ } else if (InitTy->hasPointerRepresentation()) {
----------------
sepavloff wrote:
> rsmith wrote:
> > sepavloff wrote:
> > > rjmccall wrote:
> > > > Aren't the null-pointer and integer-constant-expression checks below
> > > > already checking this? Also, `isEvaluatable` actually computes the
> > > > full value internally (as an `APValue`), so if you're worried about the
> > > > memory and compile-time effects of producing such a value, you really
> > > > shouldn't call it.
> > > >
> > > > You could reasonably move this entire function to be a method on `Expr`
> > > > that takes an `ASTContext`.
> > > Comment for `EvaluateAsRValue` says that it tries calculate expression
> > > agressively. Indeed, for the code:
> > > ```
> > > decltype(nullptr) null();
> > > int *p = null();
> > > ```
> > > compiler ignores potential side effect of `null()` and removes the call,
> > > leaving only zero initialization. `isNullPointerConstant` behaves
> > > similarly.
> > Nonetheless, it looks like this function could evaluate `Init` up to three
> > times, which seems unreasonable. Instead of the checks based on trying to
> > evaluate the initializer (`isNullPointerConstant` +
> > `isIntegerConstantExpr`), how about calling `VarDecl::evaluateValue()`
> > (which will return a potentially pre-computed and cached initializer value)
> > and checking if the result is a zero constant?
> >
> > In fact, `tryEmitPrivateForVarInit` already does most of that for you, and
> > the right place to make this change is probably in
> > `tryEmitPrivateForMemory`, where you can test to see if the `APValue` is
> > zero-initialized and produce a `zeroinitializer` if so. As a side-benefit,
> > putting the change there will mean we'll also start using `zeroinitializer`
> > for zero-initialized subobjects of objects that have non-zero pieces.
> An important point in this patch is that CodeGen tries to find out, if the
> initializer can be replaced with zeroinitializer, *prior* to the evaluation
> of it. For huge arrays the evaluation consumes huge amount of memory and time
> and it must be avoided.
>
> With this patch CodeGen may evaluate parts of the initializer, if it is
> represented by `InitListExpr`. It may cause redundant calculation, for
> instance if the check for zero initialization failed but the initializer is
> constant. To avoid this redundancy we could cache result of evaluation in
> instances of `Expr` and then use the partial values in the evaluation of the
> initializer. The simple use case solved by this patch probably is not a
> sufficient justification for such redesign.
I really think you should have some sort of type restriction in front of this
so that you don't end up creating a huge APValue only to throw it away because
it's not an int or a pointer. It's quite possible to have something like an
array initializer in a nested position that's not within an InitListExpr , e.g.
due to compound literals or std::initializer_list.
Repository:
rC Clang
https://reviews.llvm.org/D46241
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
