[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 a comment.

Group review comments.
Also, can we update the patch description to elaborate on what we mean by "strings"?



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:102
+  // vector will do.
+  std::vector<GlobalVariable *> ConstsToMerge;
+  Align MaxAlignment;
----------------
nit: Maybe renaming this is more clear? `MergeableStrings`, or something similar? 


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:118
+// sure that they can be replaced.
+static bool hasValidUsers(GlobalVariable &GV) {
+  for (User *CurrentUser : GV.users()) {
----------------
Can we rename this function so it is more clear?




================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:121
+    // Instruction users are always valid.
+    if (dyn_cast<Instruction>(CurrentUser))
+      continue;
----------------
(For here and below) Instead of `dyn_cast<>`, maybe `isa<>` is better?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:132
+
+    // We only support Intruction and Constant users.
+    if (!dyn_cast<Constant>(CurrentUser))
----------------



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:155
+
+    // If a gobal constant has a section we do not try to pool it because
+    // there is no guarantee that other constants will also be in the same
----------------



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:168
+
+    Constant *Const = Global.getInitializer();
+    ConstantDataSequential *ConstData = dyn_cast<ConstantDataSequential>(Const);
----------------
Check to see if line 168 can be inlined.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:223
+  // Use an anonymous struct to pool the strings.
+  Constant *AnonStruct = ConstantStruct::getAnon(ConstantsInStruct);
+  PooledStructType = AnonStruct->getType();
----------------

Can we add a TODO to split the struct if the offsets get too big?


================
Comment at: llvm/test/CodeGen/PowerPC/stringpool.ll:5
+; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr8 \
+; RUN:   -ppc-merge-string-pool=true -ppc-asm-full-reg-names < %s | \
+; RUN:   FileCheck %s --check-prefixes=AIX64,AIXDATA
----------------
If this is on by default, I think `-ppc-merge-string-pool=true` can be removed for here and other places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155730