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

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:188
+
+bool GISelAddressing::instMayAlias(const MachineInstr &MI,
+                                   const MachineInstr &Other,
----------------
arsenm wrote:
> This function seems to largely be recreating MachineInstr::mayAlias
There's some overlap but it doesn't look functionally the same, we're also calling out to our own analysis in this implementation as well via `aliasIsKnownForLoadStore()`.


================
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()),
----------------
arsenm wrote:
> Similar to the change in 9d720dcb89e8da4d12aa1832d74614adc6aa2c82, should this use getModRefInfo instead of isNoAlias?
Maybe, I'll take look at that.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:318
+          TLI->canMergeStoresTo(AS, StoreEVT, *MF) &&
+          (TLI->isTypeLegal(StoreEVT)/* || isTruncStoreLegal()*/))
+        break; // We can generate a MergeSize bits store.
----------------
paquette wrote:
> is the commented out code here intentional?
Nope, I'll remove that. We may support that in future though.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp:630
+    SmallVector<LegalityQuery::MemDesc, 2> MemDescrs(
+        {{Ty, Ty.getSizeInBits(), AtomicOrdering::NotAtomic}});
+    SmallVector<LLT> StoreTys({Ty, PtrTy});
----------------
arsenm wrote:
> I guess this is just ignoring alignment?
Yes, for now we generate stores with the alignment of the original store, so it's conservative.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109131