[PATCH] D155730: [PowerPC] Add a pass to merge all of the constant globals into one pool.
Amy Kwan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 27 09:44:35 PDT 2023
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
More information about the llvm-commits
mailing list