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

[Amy Kwan via Phabricator via llvm-commits]

amyk added a comment.

Submitting group code review.



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:94
+    AU.addPreserved<ScalarEvolutionWrapperPass>();
+    AU.addPreserved<SCEVAAWrapperPass>();
+  }
----------------
Can adding the constant GEP in PHI affect SCEV (line 93/94)?



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:235
+  GlobalVariable *PooledGlobal =
+      new GlobalVariable(M, ConstantPool->getType(),
+                         /* isConstant */ true, GlobalValue::PrivateLinkage,
----------------
Use `PooledStructType` in this line.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:275
+// function adds the GEP.
+void PPCMergeStringPool::insertGEPBeforeUsers(GlobalVariable *GToReplace,
+                                              GlobalVariable *GPool,
----------------
Rewording of the function for clarity.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:278-279
+                                              unsigned ElementIndex) {
+  ConstantInt *Zero = ConstantInt::get(Type::getInt32Ty(*Context), 0);
+  ConstantInt *Index =
+      ConstantInt::get(Type::getInt32Ty(*Context), ElementIndex);
----------------
Can these variables be inlined?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:285
+
+  // Need to save a temporary copy of each use list because we remove uses
+  // as we replace them.
----------------



================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:287
+  // as we replace them.
+  SmallVector<User *> UsersList;
+  for (User *CurrentUser : GToReplace->users())
----------------
Maybe just:


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:288
+  SmallVector<User *> UsersList;
+  for (User *CurrentUser : GToReplace->users())
+    UsersList.push_back(CurrentUser);
----------------
Instead of `G`, use the full word (`Global`) for all `G*` variables.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:296
+    if (UseInstruction) {
+      // GEP instructions cannot be added before PHI nodes.
+      if (PHINode *UsePHI = dyn_cast<PHINode>(UseInstruction)) {
----------------
Expand on this a bit more (that the GEP is added into the PHI itself).


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:303
+      }
+
+      GetElementPtrInst *GEPInst =
----------------
Indicate that for regular instructions we add the GEP before hand.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:321
+
+    Constant *ConstGEP = ConstantExpr::getInBoundsGetElementPtr(
+        PooledStructType, GPool, Indices);
----------------
- Add a comment here to describe the situation (adding the GEP inline).
- Also, move this before line 338 (where it is first used).


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:325-326
+
+    // At this point we expect that the user is either an instruction or a
+    // constant.
+    assert(UserConstant &&
----------------
- Clarify the comment a bit?
- Can we move all of the casts/checks/asserts (also on 331, 335) on top rather than at the bottom (so we check everything that is valid, then we go about the replacing)?
- Add an IR test for this case.


================
Comment at: llvm/test/CodeGen/PowerPC/O3-pipeline.ll:76
+; CHECK-NEXT:       FunctionPass Manager
+; CHECK-NEXT:       Dominator Tree Construction
 ; CHECK-NEXT:       Natural Loop Information
----------------
Can we check if the Dominator Tree Construction is expected, or if it's just there because it's a module pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155730