[PATCH] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: clang/lib/CodeGen/CGCXXABI.h:113
+
+  virtual bool isCXXGlobalInitAndDtorFuncInternal() const { return true; }
+
----------------
Xiangling_L wrote:
> jasonliu wrote:
> > Do we need this isCXXGlobalInitAndDtorFuncInternal? Looks like it's always going to return ! useSinitAndSterm() result.
> AFAIK, not all platforms which use sinit and sterm have external linkage sinit/sterm functions. And also since for 7.2 AIX we are considering change sinit/sterm to internal linkage symbols, I seperate this query out.
I would prefer to create the query in the patch when we actually need it. With what we have right now, it seems redundant. 


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:650
+      FTy,
+      (UseSinitAndSterm ? "__sinit80000000_clang_" : "_GLOBAL__sub_I_") +
+          FuncSuffix,
----------------
It looks like one of the UseSinitAndSterm is redundant. We could have something like: 
```
UseSinitAndSterm ? llvm::twine("__sinit80000000_clang_") + GlobalUniqueModuleId : llvm::twine("_GLOBAL__sub_I_") + getTransformedFileName(getModule())
```
which minimize the use of std::string?
If this is too long, then we could separate them into if statement just like what we have in EmitCXXGlobalDtorFunc, or use a lambda.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:692
+  if (UseSinitAndSterm) {
+    Fn = CreateGlobalInitOrDestructFunction(
+        FTy, llvm::Twine("__sterm80000000_clang_") + GlobalUniqueModuleId, FI,
----------------
Could we assert !GlobalUniqueModuleId.empty() here?
Right now, it solely relies on `EmitCXXGlobalInitFunc` to run before `EmitCXXGlobalDtorFunc` to get `GlobalUniqueModuleId` initialized properly. I think it makes sense to put an assert here to make sure it stays in that case in the future.  


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:699
+
+  if (!getCXXABI().isCXXGlobalInitAndDtorFuncInternal())
+    Fn->setLinkage(llvm::Function::ExternalLinkage);
----------------
This could go inside of the `if (UseSinitAndSterm)`.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4479
+
+  llvm::Value *NeedsDestruct = CGF.Builder.CreateIsNull(V, "guard.hasSrterm");
+
----------------
jasonliu wrote:
> Srterm is not used in this implementation.
> Suggestion:
> guard.hasDtorCalled
"guard.hasDtor" does not reflect what the meaning. Maybe matching with the variable name would make sense: "guard.needsDestruct" or just "needsDestruct"?


================
Comment at: clang/test/CodeGen/static-init.cpp:15
+
+// CHECK: define internal void @__cxx_global_var_init() #0 {
+// CHECK: entry:
----------------
#0 could be removed.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D74166