[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.

One minor comment on the code, a couple of nits on the test cases.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1987
@@ -1986,3 +1986,3 @@
     } else {
-      CmpIndVar = Builder.CreateTrunc(CmpIndVar, ExitCnt->getType(),
-                                      "lftr.wideiv");
+      // First we try to extend trip count if legal, if not
+      // we will truncate the IV
----------------
Please wrap this comment to 80 columns.  Actually, I'd be just a little bit more descriptive here about the rationale -- since `A == B` == `sext(A) == sext(B)` == `zext(A) == zext(B)`, we choose one of these encodings of the equality check if we already have a widened version of the induction variable with us.

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

================
Comment at: test/Transforms/IndVarSimplify/lftr-wide-trip-count.ll:6
@@ +5,3 @@
+
+
+define void @test1(float* nocapture %autoc,
----------------
Are the `noalias`, `nocapture`, `readnone`, `readonly` argument attributes necessary?  If not, I'd just drop them to get a more targeted test case.

================
Comment at: test/Transforms/IndVarSimplify/lftr-wide-trip-count.ll:21
@@ +20,3 @@
+  %arrayidx = getelementptr inbounds float, float* %data, i64 %idxprom
+  %1 = load float, float* %arrayidx, align 4
+  %mul = fmul float %1, %d
----------------
Please use -instnamer on these to remove the `%0` variable names.  Otherwise these tests will become difficult to edit later.

================
Comment at: test/Transforms/IndVarSimplify/lftr-wide-trip-count.ll:79
@@ +78,3 @@
+; CHECK %wide.trip.count = zext
+; %exitcond = icmp ne
+; br i1 %exitcond
----------------
Missing `CHECK:` s?

================
Comment at: test/Transforms/IndVarSimplify/lftr-wide-trip-count.ll:83
@@ +82,3 @@
+
+define float @test3(float* noalias nocapture readnone %a,
+                    float* noalias nocapture readonly %b,
----------------
Looks like `%a` is unused here and below?

================
Comment at: test/Transforms/IndVarSimplify/lftr-wide-trip-count.ll:156
@@ +155,3 @@
+; CHECK %wide.trip.count = zext
+; %exitcond = icmp ne
+; br i1 %exitcond
----------------
Missing `CHECK:` s?


https://reviews.llvm.org/D23075