[PATCH] D127392: [AggressiveInstCombine] Combine consecutive loads which are being merged to form a wider load.

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:565
+  MemoryLocation Loc = MemoryLocation::get(LI2);
+  for (Instruction &Inst : make_range(LI1->getIterator(), LI2->getIterator())) {
+    if (Inst.mayWriteToMemory() && isModSet(AA->getModRefInfo(&Inst, Loc)))
----------------
bipmis wrote:
> dmgreen wrote:
> > bipmis wrote:
> > > dmgreen wrote:
> > > > Should this have a limit on the number of instructions?
> > > Some tests like load64_farLoads() which have wider instruction gap b/w loads may result in partial combine(when tried with 16). I can possibly go for a bigger limit or can keep the limit on the actual instructions b/w 2 loads.
> > Using what limit?
> In the current implementation worst case limit could be all instructions in a BB. What is the issue with this?
> For the test case load64_farLoads() it does fine with a limit of 35. 
It protects us where there are thousands of instructions in the block, just to be safe for degenerate cases. If we expect the maximum pattern to be (load+zext+shift+or) * i8->i64, so 4*8, then a limit of 64 instructions sounds fine.


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

https://reviews.llvm.org/D127392