[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:60
+      return true;
+    if (LHS->getNumUses() < RHS->getNumUses())
+      return false;
----------------
Silly question - Does this need to be an `else if`? Just wondering since the alignments are an `else if` so maybe I am missing something.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:90
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.addPreserved<DominatorTreeWrapperPass>();
+    AU.addPreserved<LoopInfoWrapperPass>();
----------------
Question: I think we discussed this before when talking about constant merging, but how do we determine which passes to preserve again? And how does this different from the constant pool merge patch?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:111
+
+bool PPCMergeStringPool::runOnModule(Module &M) {
+  bool Changed = false;
----------------
Is there a reason why this is declared outside, rather than inside the class?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:163
+
+    LLVM_DEBUG(dbgs() << "Have constant data: ");
+    LLVM_DEBUG(ConstData->dump());
----------------
This might be just me and is a minor nit, but I think it may read better of it was something like "Constant data of Global:" or something like that.

This is minor though, so you can feel free to go with whichever one you like.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:279
+
+      // After the GEP is insterted the GV can be replaced.
+      CurrentUser->replaceUsesOfWith(GToReplace, GEPInst);
----------------



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:297
+
+    // The use was not found so it must have been replaced earlier.
+    if (!userHasOperand(UserConstant, GToReplace))
----------------
Do we mean `user` here?


================
Comment at: llvm/lib/Target/PowerPC/PPCTargetMachine.cpp:105
+    MergeStringPool("ppc-merge-string-pool",
+                    cl::desc("Merge all of strings in a module into one pool"),
+                    cl::init(false), cl::Hidden);
----------------



================
Comment at: llvm/test/CodeGen/PowerPC/stringpool.ll:1
+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr8 \
+; RUN:   -ppc-merge-string-pool=true < %s | FileCheck %s --check-prefixes=AIX32,AIXDATA
----------------
Can we add `-ppc-asm-full-reg-names` to this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155730