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