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

[Amy Kwan via Phabricator via llvm-commits]

amyk added a comment.

More group review comments.

It would be good add an IR to IR test for this patch, as well.



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:111
+
+} // namespace
+
----------------
Please check that this is where the namespace should end.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:221
+
+  // Use an anonymous struct to pool the strings.
+  // TODO: This pass uses a single anonymous struct for all of the pooled
----------------
Can we reword the comment (to describe the intention behind the code and to elaborate on why we're doing it)?

Also, update the TODO below to reflect that we're calling it "pool" rather than "struct".


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:226
+  // future we may want to split this into multiple structs.
+  Constant *AnonStruct = ConstantStruct::getAnon(ConstantsInStruct);
+  PooledStructType = AnonStruct->getType();
----------------
Can we also update the naming for AnonStruct?
Maybe just ConstantPool?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:230
+  // The GlobalVariable constructor calls MM->insertGlobalVariable(G).
+  GlobalVariable *G =
+      new GlobalVariable(M, AnonStruct->getType(),
----------------
Rename this maybe so we know what the global is for?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:247-251
+    // Need to save a temporary copy of each use list because we remove uses
+    // as we replace them.
+    SmallVector<User *> UserList;
+    for (User *CurrentUser : GV->users())
+      UserList.push_back(CurrentUser);
----------------

This whole part can probably be put into the function that inserts the GEP before the users.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:253
+
+    insertGEPBeforeUser(UserList, GV, G, ElementIndex);
+
----------------
- This function name update might be more clear.
- Add a comment as to why we need to insert the GEPs.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:256
+    // This GV has no more uses and is privately linked so we can erase it.
+    if (GV->use_empty() && GV->getLinkage() == GlobalValue::PrivateLinkage)
+      GV->eraseFromParent();
----------------
- Maybe it would be safer to limit the globals only to private linkage (move the check to `collectCandidateConstants`)?
- Can this either be either private or internal linkage?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155730