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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
