[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D76096#4533497 <https://reviews.llvm.org/D76096#4533497>, @efriedma wrote:

> Basically, I don't want order-of-magnitude compile-time regressions with large global variables.  There are basically two components to that:
>
> - That the fast path for emitting globals in C continues be fast.

https://reviews.llvm.org/D76096#4529555 had some numbers for the linux kernel.

Perhaps playing with time to compile programs generated via incbin <https://github.com/graphitemaster/incbin> would be a useful test, too?

> - That we rarely end up using evaluateValue() on large global arrays/structs in C, for reasons other than emitting them.  If we end up calling evaluateValue() on most globals anyway, the codegen fast-path is a lot less useful;

But prior to D151587 <https://reviews.llvm.org/D151587>, we did that for C++. Why is C special here?

And prior to this patch, we did that for C++ 11+. Why is C++ 03 special here?

To find such cases where the CGExprConstant "fast path" is failing, I'm adding logging to the two cases from D151587 <https://reviews.llvm.org/D151587> where the fast path fails but the slow path succeeds, then building the Linux kernel:

  diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
  index 9ad07f7d2220..6b618db281bd 100644
  --- a/clang/lib/CodeGen/CGExprConstant.cpp
  +++ b/clang/lib/CodeGen/CGExprConstant.cpp
  @@ -1677,8 +1683,14 @@ llvm::Constant *ConstantEmitter::tryEmitPrivateForVarInit(const VarDecl &D) {
   
     // Try to emit the initializer.  Note that this can allow some things that
     // are not allowed by tryEmitPrivateForMemory alone.
  -  if (APValue *value = D.evaluateValue())
  -    return tryEmitPrivateForMemory(*value, destType);
  +  if (APValue *value = D.evaluateValue()) {
  +    llvm::Constant *C = tryEmitPrivateForMemory(*value, destType);
  +    if (C) {
  +      llvm::dbgs() << "ConstExprEmitted failed (but EvaluateValue succeeded):\n";
  +      E->dump();
  +    }
  +    return C;
  +  }
   
     return nullptr;
   }
  @@ -1760,8 +1772,14 @@ llvm::Constant *ConstantEmitter::tryEmitPrivate(const Expr *E,
     else
       Success = E->EvaluateAsRValue(Result, CGM.getContext(), InConstantContext);
   
  -  if (Success && !Result.HasSideEffects)
  -    return tryEmitPrivate(Result.Val, destType);
  +  if (Success && !Result.HasSideEffects) {
  +    llvm::Constant *C = tryEmitPrivate(Result.Val, destType);
  +    if (C) {
  +      llvm::dbgs() << "ConstExprEmitter failed (but Evaluate succeeded):\n";
  +      E->dump();
  +    }
  +    return C;
  +  }
   
     return nullptr;
   }

There are still a lot of cases that could be improved in the fast path. DeclRefExpr seems error prone/more difficult, but there are still lots of stupid/simple cases (like `int x = -1;` being a UnaryOperator that currently isn't handled, and `long x = 1` having a widening ImplicitCast, `short x = 1` having a narrowing implicit cast, IIRC `sizeof` failed in the fast path even when not using DeclRefExpr).   But then when I go to improve the "fast path" you give me feedback that <https://reviews.llvm.org/D156185#4530117>:

>> We can do a whole bunch of these, but at some point we hit diminishing returns... bailing out here isn't *that* expensive. Do you have some idea how far you want to go with this? Adding a couple obvious checks like this isn't a big deal, but I don't want to build up a complete parallel infrastructure for scalar expressions here.

So I feel like your feedback is pulling me in opposite directions.  You want to avoid the fast path falling back to the slow path, but improvements to the fast path directly result in "complete parallel infrastructure" which you also don't want.  Those seem mutually exclusive to me. Is there a third option?  Am I attacking a strawman? (Regardless, I don't want to seem unappreciative of the reviews, advice, or discussion).

> most of the cost of the APValue codepath is generating the APValue, not lowering it.

That is my understanding of the "slow paths" as well.  I think I'll send a patch with just comments explaining this (which is really a follow up to D151587 <https://reviews.llvm.org/D151587> more so than this patch).

> This is not quite as concrete as I'd like, but I'm not sure how to describe it.

D151587 <https://reviews.llvm.org/D151587> is in release/17.x and release/17.x has now branched. I'd like to try to get this patch cherry picked back to release/17.x, and as such I have marked https://github.com/llvm/llvm-project/issues/44502 as part of the release/17.x milestone.  Please let me know if you think that's too aggressive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76096