[PATCH] D148286: [PowerPC] Merge the constant pool on Power PC AIX.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineConstantPool.h:22
 #include <climits>
+#include <set>
 #include <vector>
----------------
Prefer llvm set types 


================
Comment at: llvm/include/llvm/CodeGen/MachineConstantPool.h:152-153
+    bool RemovedSomething = false;
+    // CPI should contain a sorted list of constant pool indices to be removed.
+    // We traverse it backwards so that we do not delete the wrong entries.
+    for (auto it = CPI.crbegin(); it != CPI.crend(); ++it) {
----------------
Why use a set at all if it's sorted? Also pass by value?


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeConstPoolEntries.cpp:262-267
+// Get a copy of the CVConst that is not marked as "const".
+// This is required because the creation of a constant struct using
+// ConstantSctruct::getAnon(..) requires ArrayRef<Constant *> and not
+// ArrayRef<const Constant *>.
+Constant *PPCMergeConstPool::getEditableConstant(const Constant *CVConst) {
+  Constant *CV = nullptr;
----------------
Seems like a workaround that shouldn't require chaining through all the constant types


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeConstPoolEntries.cpp:298
+  uint64_t IndexOfMergedPool = 0;
+  for (auto &ConstVal : ModuleEntriesList) {
+    unsigned AlignValue = ConstVal.second.Alignment.value();
----------------
tuple decompose syntax 


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeConstPoolEntries.cpp:329
+    Type *ElemType = C->getType();
+    ByteOffsetInStruct += DL.getTypeSizeInBits(ElemType) / 8;
+  }
----------------
getTypeStoreSize


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeConstPoolEntries.cpp:356-359
+    LLVM_DEBUG(dbgs() << "Constant Offset: " << ConstVal.second.OffsetIntoStruct
+                      << "\n");
+    LLVM_DEBUG(dbgs() << "For Constant: ");
+    LLVM_DEBUG(C->dump());
----------------
Can merge these into one LLVM_DEBUG block


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeConstPoolEntries.cpp:377-379
+      LLVM_DEBUG(
+          dbgs() << "mergeConstantsForModule: Replacing this instruction:\n");
+      LLVM_DEBUG(MI->dump());
----------------
Can directly dbgs() << *MI in the first print


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148286