[PATCH] D46241: [CodeGen] Recognize more cases of zero initialization

[John McCall via Phabricator via cfe-commits]

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