[PATCH] D109131: [GlobalISel] Add a store-merging optimization pass and enable for AArch64.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h:38
+  Register IndexReg;
+  Optional<int64_t> Offset;
+  bool IsIndexSignExt = false;
----------------
Why Optional? Isn't 0 naturally good enough?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:65
+  MRI = &MF.getRegInfo();
+  TTI = &getAnalysis<TargetTransformInfoWrapperPass>().getTTI(MF.getFunction());
+  AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
----------------
Don't know what you would want TTI for in a MIR pass, but this is unused


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:114
+
+  if (!(BasePtr0.BaseReg.isValid() && BasePtr1.BaseReg.isValid()))
+    return false;
----------------
Demorgan this?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:188
+
+bool GISelAddressing::instMayAlias(const MachineInstr &MI,
+                                   const MachineInstr &Other,
----------------
This function seems to largely be recreating MachineInstr::mayAlias


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:197
+    int64_t Offset;
+    Optional<int64_t> NumBytes;
+    MachineMemOperand *MMO;
----------------
Optional seems like overkill, 0 size is naturally a missing size


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:271
+    int64_t Overlap1 = *Size1 + SrcValOffset1 - MinOffset;
+    if (AA->isNoAlias(MemoryLocation(MUC0.MMO->getValue(), Overlap0,
+                                     MUC0.MMO->getAAInfo()),
----------------
Similar to the change in 9d720dcb89e8da4d12aa1832d74614adc6aa2c82, should this use getModRefInfo instead of isNoAlias?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:298
+  const auto &LegalSizes = LegalStoreSizes[AS];
+  const auto &TLI = MF->getSubtarget().getTargetLowering();
+
----------------
Cache this in the class?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:305
+
+  auto DL = MF->getFunction().getParent()->getDataLayout();
+  bool AnyMerged = false;
----------------
This should be a const reference


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:337
+                                            MachineFunction &MF) const {
+  const auto &LI = MF.getSubtarget().getLegalizerInfo();
+  auto Action = LI->getAction(Query).Action;
----------------
Cache this in the class?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:381
+  auto *WideMMO = MF->getMachineMemOperand(&Stores[0]->getMMO(), 0,
+                                           WideValueTy.getSizeInBytes());
+  if (!ConstantVals.empty()) {
----------------
Should directly pass WideValueTy, the memory operands track the type now


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:396-399
+    // Mimic the SDAG behaviour here and don't try to do anything for unknown
+    // values. In future, we should also support the cases of loads and
+    // extracted vector elements.
+    return false;
----------------
Invert condition and unindent the if case?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:624
+  const auto &LI = *MF->getSubtarget().getLegalizerInfo();
+  LLT PtrTy = LLT::pointer(AddrSpace, 64);
+  // We assume that we're not going to be generating any stores wider than
----------------
Assuming the pointer size is 64, need to query for this


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:630
+    SmallVector<LegalityQuery::MemDesc, 2> MemDescrs(
+        {{Ty, Ty.getSizeInBits(), AtomicOrdering::NotAtomic}});
+    SmallVector<LLT> StoreTys({Ty, PtrTy});
----------------
I guess this is just ignoring alignment?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:647-649
+  // Don't run the pass if the target asked so.
+  if (DoNotRunPass(MF))
+    return false;
----------------
Wouldn't the target just not add the pass to the pipeline?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109131