[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.
This revision now requires changes to proceed.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:100
@@ +99,3 @@
+
+  OriginalIVInfo() {};
+
----------------
Why do you need this constructor?

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:104
@@ +103,3 @@
+
+typedef ValueMap<Value *, std::unique_ptr<OriginalIVInfo>> WidenedIVMapType;
+
----------------
Why not have the LHS be an `AssertingVH`?

Actually, will keeping a raw pointer (or an `AssertingVH`) work if an IV is widened twice?

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1432
@@ +1431,3 @@
+                  new OriginalIVInfo(AddRec->hasNoUnsignedWrap(),
+                                     AddRec->hasNoSignedWrap(),
+                                     IsSigned,
----------------
Use `make_unique` here.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2014
@@ -1986,3 +2013,3 @@
     } else {
-      CmpIndVar = Builder.CreateTrunc(CmpIndVar, ExitCnt->getType(),
-                                      "lftr.wideiv");
+      bool extended = false;
+      const auto Iter = WidenedIVMap.find(IndVar);
----------------
s/extended/Extended/ to follow the coding conventions

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2017
@@ +2016,3 @@
+      if (Iter != WidenedIVMap.end() &&
+         ((Iter->second)->OriginalType == ExitCnt->getType())) {
+        if (Iter->second->HasNSW && Iter->second->IsSigned) {
----------------
Not sure if you need the braces around `Iter->second`.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2023
@@ +2022,3 @@
+        }
+        if (Iter->second->HasNUW && !Iter->second->IsSigned) {
+          extended = true;
----------------
I think there is a way to make this stateless (though I don't know if SCEV is smart enough to make that work viably for all the cases you care about): you were going to emit a backedge condition of `trunc(IV) == ExitCnt`.  (Please double check this logic) since `sext` and `zext` are one-to-one, this is equivalent to both `zext(trunc(IV)) == zext(ExitCnt)` and `sext(trunc(IV)) == sext(ExitCnt)`.

`zext(trunc(IV)) == zext(ExitCnt)` is profitable if `zext(trunc(IV))` == `IV` (in which case the RHS is `zext(ExitCnt)`) and `sext(trunc(IV)) == sext(ExitCnt)` is profitable if `sext(trunc(IV))` == `IV` (in which case the RHS is `sext(ExitCnt)`).  You can check these equivalences (i.e. `exit(trunc(T))` == `T`) using SCEV.

I think this will be a better solution overall, if it works. :)


https://reviews.llvm.org/D23075