[PATCH] D155730: [PowerPC] Add a pass to merge all of the constant globals into one pool.

[Amy Kwan via Phabricator via llvm-commits]

amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:78
+  }
+} CompareConstants;
+
----------------
New group code review questions:
- Have we considered alignment based string pool struct?
- Have we considered making this target independent and making a Discourse discussion regarding this?



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:192
+
+  SmallVector<Constant *> OrderedVector;
+  for (GlobalVariable *GV : ConstsToMerge) {
----------------
Could we update the name of this SmallVector to better indicate it's purpose?



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:210
+
+  unsigned ElementIndex = 0;
+  for (GlobalVariable *GV : ConstsToMerge) {
----------------
Is our element index big enough?
Should it be `size_t`, or at least a size large enough for what we are targeting? It should support any platform's `size_t`.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:242
+}
+
+void PPCMergeStringPool::insertGEPBeforeUser(SmallVector<User *> &UsersList,
----------------
Could we please add some documentation to this function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155730