[PATCH] D23075: [IndVarSimplify] Extend trip count instead of truncating IV in LFTR, when original IV does not overflow

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Code-wise only minor comments, otherwise the code lgtm.

Test-wise, you really need a few specific test cases for the code you've added (and one or two negative tests).  Even if the code is being exercised as part of other tests, it is nicer to have commits come with test cases that document what new functionality was added.

You also need to not use grep in the test cases.  Note: I've fixed all of the IndVarSimplify tests to not use grep.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1987
@@ -1986,3 +1986,3 @@
     } else {
-      CmpIndVar = Builder.CreateTrunc(CmpIndVar, ExitCnt->getType(),
-                                      "lftr.wideiv");
+      bool Extended = false;
+      const SCEV *IV = SE->getSCEV(CmpIndVar);
----------------
I'd s/`Extended`/`ExtendedIV`/

Also add a one-line comment on what you're trying to do: instead of truncating the IV, try to extend the exit count, if legal.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1989
@@ +1988,3 @@
+      const SCEV *IV = SE->getSCEV(CmpIndVar);
+      const SCEV *ZExTrunc =
+           SE->getZeroExtendExpr(SE->getTruncateExpr(SE->getSCEV(CmpIndVar),
----------------
I'd s/`ZExTrunc`/`ZExtTrunc`/

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2010
@@ +2009,3 @@
+
+      if (!Extended) {
+        CmpIndVar = Builder.CreateTrunc(CmpIndVar, ExitCnt->getType(),
----------------
No braces needed here.


https://reviews.llvm.org/D23075