[PATCH] D47928: [SimplifyIndVars] Eliminate redundant truncs

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked 2 inline comments as done.
mkazantsev added inline comments.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:555
+  // Replace all comparisons against trunc with comparisons against IV.
+  BasicBlock *Predecessor = L->getLoopPredecessor();
+  for (auto *ICI : ICmpUsers) {
----------------
reames wrote:
> I think you're better off placing these in the loop and then letting existing code hoist.  Specifically, this code hoists about a possible condition in the predecessor (i.e. when it's not a preheader) and while that's legal, it might not be always profitable.  
> 
> You can handle the simple cases canonically by placing in the loop and calling L->makeLoopInvariant(I) on them.
Ok, let us follow "don't make worse" strategy and insert new comparisons in location of old ones.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:572
+    ICI->replaceAllUsesWith(NewICI);
+    ICI->eraseFromParent();
+  }
----------------
We should not erase instructions here. It should be added to DeadInsts instead. Potential memory corruption (I don't have such test, though).


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:574
+  }
+
+  // Trunc no longer needed.
----------------
reames wrote:
> This is fine for the moment, but I want to note a couple of possible follow-ups:
> 1) We could update signed users even if we can't updated unsigned users or vice versa.
> 2) We could allow unrelated users.
> 3) If we encounter a non-canonical signed compare (i.e. one which could be represented as a unsigned compare), maybe we should emit the zext form directly?
> 
> These are explicitly out of scope for this patch.
Ok, I will add a comment on that.


https://reviews.llvm.org/D47928