[PATCH] D155688: [PATCH] [llvm] [InstCombine] Reassociate loop invariant GEP index calculations.

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2336
+    // Try to reassociate loop invariant index calculations to enable LICM.
+    if (Idx && (Idx->getOpcode() == Instruction::Add)) {
+      Value *Ptr = GEP.getOperand(0);
----------------
This needs a one-use check. The transform is not profitable if we have to keep *both* the add and the gep.

Can also use `match(GEP.getOperand(1), m_Add(...))` here.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2339
+      Value *InvIdx = Idx->getOperand(0);
+      Value *NonInvIdx = Idx->getOperand(1);
+
----------------
This no longer checks for loop invariance, so we should remove any invariance-related terminology.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2351
+                                               NewPtr, NonInvIdx);
+      NewGEP->setIsInBounds(GEP.isInBounds());
+      return NewGEP;
----------------
This inbounds preservation is incorrect: https://alive2.llvm.org/ce/z/bJZvQG

It's even incorrect if the add is also nsw.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155688