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

Xiangling Liao via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 6 12:04:19 PDT 2020


Xiangling_L marked 5 inline comments as done.
Xiangling_L added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:466
+
+  bool preprocessStructorList(const DataLayout &DL, const Constant *List,
+                              SmallVector<Structor, 8> &Structors);
----------------
jasonliu wrote:
> 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. 
I think we do have a `XXStructorList` here which is the initializer of `llvm.gloal_ctors`or `llvm.gloal_dtors` array? The usage of this term is  consistent with other spots.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2107
+  if (!isa<ConstantArray>(List))
+    return false;
 
----------------
jasonliu wrote:
> 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?
Yeah, we could do that. But it looks a boolean return value makes the code flow natural. And if any target does want to control the early return in the future, it's flexbile for them to do that. I am wondering is there any big difference between bool and void return value for this function? 


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

https://reviews.llvm.org/D84534



More information about the cfe-commits mailing list