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.



cfe-commits mailing list

Reply via email to