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

[Lei Huang via Phabricator via llvm-commits]

lei added a comment.

For new passes, maybe we can commit it as off first and then subsequent patch to turn it on by default?  That way if it causes any issues we can just revert the default on patch.



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:213
+  // candidates we can skip doing the merging.
+  if (MergeableStrings.size() <= 1)
+    return false;
----------------
Wondering if maybe we need to do more investigation into this, as in is there any negative benefits to merging strings if they are < N.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:222
+    ConstantsInStruct.push_back(GV->getInitializer());
+  }
+
----------------
nit: Do we need the braces for the for-loop?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:305-335
+    if (UserInstruction) {
+      if (PHINode *UserPHI = dyn_cast<PHINode>(UserInstruction)) {
+        // GEP instructions cannot be added before PHI nodes.
+        // With getInBoundsGetElementPtr we create the GEP and then replace it
+        // inline into the PHI.
+        Constant *ConstGEP = ConstantExpr::getInBoundsGetElementPtr(
+            PooledStructType, GPool, Indices);
----------------
Was thinking maybe we can split this nested if into an early exit of the loop.
Basically:
```
if (!UserInstruction) {
  // User is a constant type.
  ....
  continue;
}

// User is a valid instruction.

if (PHINode *UserPHI = dyn_cast<PHINode>(UserInstruction)) {
  // User is a PHI node instruction.
  .....
  continue;

}

// The user is a valid instruction that is not a PHINode.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:313
+        UserPHI->replaceUsesOfWith(GlobalToReplace, ConstGEP);
+	continue;
+      }
----------------
nit: indent


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155730