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

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.


================
Comment at: format:1
+//===-- PPCAsmPrinter.cpp - Print machine instrs to PowerPC assembly ------===//
+//
----------------
Redundant file?


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:455
   }
 
+  struct Structor {
----------------
The new block is not all `Overridable Hooks`, seems better belong in `Coarse grained IR lowering routines`.


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:456
 
+  struct Structor {
+    int Priority = 0;
----------------
This struct got moved to header, means it's not an implementation detail any more. 
We would need doxygen comment on it.


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:460
+    // In most cases, `Key` represents a `ComdatKey`. Except on AIX, a `Key` is
+    // used to identify a csect where we should emit `.ref` when needed.
+    GlobalValue *Key = nullptr;
----------------
I have a slight preference to leave it as ComdatKey, and change the name when we actually need to handle `.ref` because without seeing the actual implementation for `.ref` we don't know if this is the way we want to pursue. 


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:466
+
+  bool preprocessStructorList(const DataLayout &DL, const Constant *List,
+                              SmallVector<Structor, 8> &Structors);
----------------
A doxygen comment describe what this function does, and what its return value means, and mention `Structors` is an output argument.
By looking at what this function does, it seems `buildStructorList` is a better name.


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:468
+                              SmallVector<Structor, 8> &Structors);
+  /// Targets can override this to change how `llvm.global_ctors` and
+  /// `llvm.global_dtors` lists get handled.
----------------
Add a blank line above this.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-static-init-key-object.ll:6
+
+ at llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @foo, i8* @v}]
+
----------------
Adding this test case would looks like we already decided how to handle .ref in clang and llvm. But in fact, we haven't. I would prefer not having this test.


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

https://reviews.llvm.org/D84534