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 

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 

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.



cfe-commits mailing list

Reply via email to