[PATCH] D67234: [MergedLoadStoreMotion] Sink stores if they have common GEP

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Broadly, this seems like a small enough change that it would be worth taking for now, even if we expect the pass to become obsolete eventually.  But there isn't much point if you aren't seeing any measurable benefit.  Could you give some idea how frequently this triggers?



================
Comment at: llvm/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:284
+  auto *A0 = dyn_cast<GetElementPtrInst>(S0->getPointerOperand());
+  auto *A1 = dyn_cast<GetElementPtrInst>(S1->getPointerOperand());
+  return A0 && A1 && A0 == A1;
----------------
Why does it matter here if the address is a GEP, as opposed to some other Value?

Instead of checking equality, could we check if the pointers are MustAlias?  Would that have any significant impact on how frequently the transform triggers?


================
Comment at: llvm/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:296
+             dbgs() << "Store Right\n"; S1->dump(); dbgs() << "\n");
+  BasicBlock::iterator InsertPt = BB->getFirstInsertionPt();
+  // Intersect optional metadata.
----------------
Is there a check somewhere that ensures InsertPt is actually a legal insertion point?  You can't insert instructions into a catchswitch.


================
Comment at: llvm/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:299
+  S0->andIRFlags(S1);
+  S0->dropUnknownNonDebugMetadata();
+
----------------
Do we need to do something with the store's alignment here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67234