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

More group code review comments.

Also, actually, is it more accurate to call this "array pooling" rather than "string pooling"?



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:49
+    // issue with incorrect alignent that would require padding.
+    Align LHSAlign = LHS->getAlign().valueOrOne();
+    Align RHSAlign = RHS->getAlign().valueOrOne();
----------------
Do we need to check size and alignment together?



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:57
+    // Next priority is the number of uses.
+    // Theoretically smaller offsets are easier to materialize.
+    if (LHS->getNumUses() > RHS->getNumUses())
----------------
Can this be slightly more elaborated upon?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:113
+  bool Changed = false;
+  Context = &M.getContext();
+
----------------
Can we get the context later on when we need it?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:119
+
+static bool hasValidUsers(GlobalVariable &GV) {
+  for (User *CurrentUser : GV.users()) {
----------------
Can we document this function (and why instruction and non-GlobalValue constant users are valid)?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:126
+    if (Constant *UserConstant = dyn_cast<Constant>(CurrentUser)) {
+      // We can replace all Constant users except GlobalValues.
+      if (dyn_cast<GlobalValue>(UserConstant))
----------------
Question:
- Can we elaborate why we can't have GlobalValues here?
- Can we add a test case where we have a constant using constant?
- Can we add a test where a constant is a global value and where it is not?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:127
+      // We can replace all Constant users except GlobalValues.
+      if (dyn_cast<GlobalValue>(UserConstant))
+        return false;
----------------
- Can we do `CurrentUser` here instead of `UserConstant` since we are casting twice?
- And can we pull this statement out after after 122-123 since we're reducing nesting?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:136
+
+void PPCMergeStringPool::collectCandidateConstants(Module &M) {
+  for (GlobalVariable &Global : M.globals()) {
----------------
Can we document this function?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:144
+
+    // We can only pool constants.
+    if (!Global.isConstant() || !Global.hasInitializer())
----------------
Is the `hasInitializer()` check necessary?

Also, is it possible to have a constant that is `undef`?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:147
+      continue;
+
+    if (Global.hasSection())
----------------
- Can we document why the global cannot have a section and metadata?
- Can we combine these into `||`? 


================
Comment at: llvm/test/CodeGen/PowerPC/stringpool.ll:31
+; AIX32-NEXT:    stw 0, 72(1)
+; AIX32-NEXT:    addi 3, 3, 62
+; AIX32-NEXT:    bl .callee[PR]
----------------
Can we add a test where we do `addis`->`addi`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155730