[PATCH] D34992: Emit static constexpr member as available_externally definition

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2293
 /// If ExcludeCtor is true, the duration when the object's constructor runs
-/// will not be considered. The caller will need to verify that the object is
-/// not written to during its construction.
+/// will not be considered (unless trivial). The caller will need to verify that
+/// the object is not written to during its construction.
----------------
This comment change suggests that when `ExcludeCtor` is true, a trivial constructor is considered. That's the opposite of what this change does -- namely, a trivial constructor is not considered regardless of the value of `ExcludeCtor`.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2302
+            Context.getBaseElementType(Ty)->getAsCXXRecordDecl())
+      return (ExcludeCtor || Record->hasTrivialDefaultConstructor()) &&
+             !Record->hasMutableFields() && Record->hasTrivialDestructor();
----------------
This change looks wrong: you don't know that the default constructor would be used to initialize the object in question, so whether the default constructor is trivial seems like it should have no bearing on the result.

I think you can change the code below that uses `GV->isConstant()` to call `isTypeConstant` instead, and remove the need for any changes here.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2433
+    // Check if we a have a const declaration with an initializer, we may be
+    // able to emit it as available_externally to expose it's value to the
+    // optimizer.
----------------
Typo "it's" -> "its"


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2435
+    // optimizer.
+    if (GV->hasExternalLinkage() && GV->isConstant() && !GV->hasInitializer() &&
+        !D->hasDefinition() && D->hasInit()) {
----------------
Instead of `GV->isConstant()`, it would make more sense to use `isTypeConstant(D->getType(), true)` here.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2437-2445
+      bool HasMutableField = false;
+      if (Context.getLangOpts().CPlusPlus) {
+        if (const CXXRecordDecl *Record =
+                Context.getBaseElementType(D->getType())->getAsCXXRecordDecl())
+          HasMutableField = Record->hasMutableFields();
+      }
+
----------------
This duplicates much of the work done by `isTypeConstant`.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2448-2451
+        if (InitExpr) {
+          GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+          GV->setInitializer(EmitConstantInit(*InitDecl));
+        }
----------------
In order for this transformation to be correct, you need to know that the variable has static initialization, which means that it needs to formally have a constant initializer. You can use `D->isInitKnownICE()` to check this.


https://reviews.llvm.org/D34992