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