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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:391
+  /// @param[out] Structors Sorted Structor structs by Priority.
+  /// @return false if List is not an array of '{ i32, void ()*, i8* }' structs.
+  bool preprocessXXStructorList(const DataLayout &DL, const Constant *List,
----------------
This description is not entirely true. We only see if the array is a ConstantArray and returning false. We are not returning false if the array's element is not `{ i32, void ()*, i8* }`.


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:466
+
+  bool preprocessStructorList(const DataLayout &DL, const Constant *List,
+                              SmallVector<Structor, 8> &Structors);
----------------
Xiangling_L wrote:
> jasonliu wrote:
> > 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.
> I meant to and should've named this function to `preprocessXXStructorList`, let me know if you still prefer `buildStructorList`. And if you do, since the underneath of `SmallVector` is a variable-sized array, maybe we should try `buildSortedStructorArray`?
`preprocess` sounds like we are already having a XXStructorList and now we try to do something on it. 
But right now, we are actually passing in an empty StructorList/Array and build it from scratch. So I would still prefer the name of `build` in it.
I don't mind changing to a more accurate name as you suggested. 


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2107
+  if (!isa<ConstantArray>(List))
+    return false;
 
----------------
Return of boolean seems unnecessary. 
Callee could check the size of the Structors to decide if they want an early return or not (or in this particular case, the for loop would just do nothing and no need for extra condition if you don't mind the call to getPointerPrefAlignment or assign to 0 to Index)?
So we could just return void for this function?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2129
   }
-
   // Emit the function pointers in the target-specific order
   llvm::stable_sort(Structors, [](const Structor &L, const Structor &R) {
----------------
nit: Missing blank line.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1966
+
+    Index++;
+  }
----------------
nit: Use `++Index` instead. 
https://llvm.org/docs/CodingStandards.html#prefer-preincrement

Or use `Index++` at line 1963 so that we don't need this line.


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

https://reviews.llvm.org/D84534