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