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

[Xiangling Liao via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4609
+    // their own llvm.global_dtors entry.
+    CGM.AddCXXStermFinalizerToGlobalDtor(StermFinalizer, 65535);
+  else
----------------
jasonliu wrote:
> Handling template instantiation seems fairly orthogonal to "moving naming logic from frontend to backend". Could we put it in a separate patch (which could be a child of this one)? 
The reason I chose to handle template instantiation and inline variable in this patch is that I want to show the scenarios where we have separate initialization. And this is also the reason why we want to embed array index into sinit/sterm functions. That is, if we move this part into a separate patch, I am worried that ppl will feel it's weird to embed index into function names.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2165
+      emitXXStructor(DL, S.Priority, index, S.Func, isCtor);
+      ++index;
+      continue;
----------------
jasonliu wrote:
> Maybe a naive quesiton, in what situation would we have name collision and need `index` as suffix to differentiate?
> Could we just report_fatal_error in those situation?
As far as I know, there are several situations we would have separate initialization without considering priority:
1) template specialization
2) inline variable
3) ctor/dtor attribute functions

By embedding the index, we can group those sinit/sterm with same priority within current file together.

And as I mentioned above, I chose to handle the former two cases in this patch to show why we need to embed the index.

And regarding the third scenario, we've already report_fatal_error on those cases.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1848
+    // beforing emitting them.
+    if (isSpecialLLVMGlobalArrayForStaticInit(&G)) {
+      if (GlobalUniqueModuleId.empty()) {
----------------
jasonliu wrote:
> This would have conflict with D84363. You might want to rebase it later. 
Thanks for the reminder, I will update the patch accordingly.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1935
+  Function *Func = cast<Function>(CV);
+  Func->setLinkage(GlobalValue::ExternalLinkage);
+  Func->setName((isCtor ? llvm::Twine("__sinit") : llvm::Twine("__sterm")) +
----------------
jasonliu wrote:
> Changing Function name and linkage underneath looks a bit scary. People would have a hard time to map IR to final assembly that gets produced. Have you thought about creating an alias (with the correct linkage and name) to the original function instead?
Good point. I will adjust the implementation here.


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

https://reviews.llvm.org/D84534