[PATCH] D47928: [SimplifyIndVars] Eliminate redundant truncs

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/comments addressed before landing.



================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:523
+  // legal.
+  bool IsSExtLegal = false;
+  bool IsZExtLegal = false;
----------------
Naming wise, including Legal is bit confusing.  Maybe DoesSExtCollapse or IsSExtPrpfitable?


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:530
+
+  // If neither sext nor zext is legal, bail.
+  if (!IsSExtLegal && !IsZExtLegal)
----------------
Again, legal here is odd since as you say at the top, it's always legal to extend both sides.  


================
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) {
----------------
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.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:574
+  }
+
+  // Trunc no longer needed.
----------------
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.


https://reviews.llvm.org/D47928