[PATCH] D84534: [AIX] Static init frontend recovery and backend support

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L marked 6 inline comments as done.
Xiangling_L added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1058
+  /// Add an sterm finalizer to its own llvm.global_dtors entry.
+  void AddCXXStermFinalizerToGlobalDtor(llvm::Function *StermFinalizer,
+                                        int Priority) {
----------------
jasonliu wrote:
> This wrapper seems redundant. Calling ` AddGlobalDtor(StermFinalizer, Priority);` directly from the callee side already convey what we are trying to achieve here. 
I added this wrapper function to keep the symmetry between `AddGlobalCtor` and `AddGlobalDtor`. They are private functions within `CodeGenModule` class. And I feel it's kinda weird to only move `AddGlobalDtor` to a public function.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4602
+    llvm::report_fatal_error(
+        "prioritized __sinit and __sterm functions are not yet supported");
+  else if (isTemplateInstantiation(D.getTemplateSpecializationKind()) ||
----------------
jasonliu wrote:
> Could we trigger this error? If so, could we have a test for it? 
I should've put an assertion here. The init priority attribute has been disabled on AIX in the previous patch.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1874
+      }
+      emitSpecialLLVMGlobal(&G);
+      continue;
----------------
jasonliu wrote:
> Have some suggestion on structure backend code change:
> 
> 1. Remove `isSpecialLLVMGlobalArrayForStaticInit` and `isSpecialLLVMGlobalArrayToSkip`, and call `emitSpecialLLVMGlobal` directly instead. This would handle all the `llvm.` variable for us. We would need do early return for names start with `llvm.` in emitGlobalVariable as well, since they are all handled here.
> 
> 2. We could override emitXXStructorList because how XCOFF handle the list is vastly different than the other target. We could make the common part(i.e. processing and sorting) a utility function, say "preprocessStructorList".
> 
> 3. It's not necessary to use the same name `emitXXStructor` if the interface is different. It might not provide much help when we kinda know no other target is going to use this interface. So we could turn it into a lambda inside of the overrided emitXXStructorList, or simply no need for a new function because the logic is fairly straightforward.
> 
1. As we discussed offline, we would leave the handling of `llvm.used` and `llvm.metadata` later and don't include them in the scope of this patch.


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

https://reviews.llvm.org/D84534