[PATCH] D24280: [IndVarSimplify] Prefer sext over zext when widening IV if it is non-negative and has a GEP user

[Sanjoy Das via llvm-commits]

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

Some comments inline.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:902
@@ -899,1 +901,3 @@
+  // Value: whether this instruction is widened by sext.
+  DenseMap<Instruction*, bool> ExtendKindMap; 
 
----------------
Please name the name so that the intent of the `bool` is obvious (perhaps `WasSignExtended`?).  Or (even better) use something other than a `bool` as the domain (perhaps an enum?). Also, clang-format.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:937
@@ -932,1 +936,3 @@
+  const SCEVAddRecExpr* getExtendedOperandRecurrence(NarrowIVDefUse DU,
+                                                     bool &WidenBySext);
 
----------------
I'm not a big fan of returning via params like these -- it is too easy to forget to update the return value along one path.  Have you considered returning an `std::pair` or even a hand-rolled struct instead?

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:946
@@ -939,1 +945,3 @@
 
+  bool canWidenBySext(NarrowIVDefUse DU);
+
----------------
Document these please.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1160
@@ -1148,3 +1159,3 @@
     cast<OverflowingBinaryOperator>(DU.NarrowUse);
-  if (IsSigned && OBO->hasNoSignedWrap())
+  if (ExtendKindMap[DU.NarrowDef] && OBO->hasNoSignedWrap())
     ExtendOperExpr = SE->getSignExtendExpr(
----------------
Common these three `ExtendKindMap[DU.NarrowDef]` lookups into one with a descriptive name.

Actually, these should not look up the DenseMap directly like these -- if `DU.NarrowDef` is not in `ExtendKindMap` then we'll get a bogus false result here.  You should add an accessor that will trip an assert if `DU.NarrowDef` is not present in `ExtendKindMap`.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1195
@@ -1181,3 +1194,3 @@
 /// so, return the sign or zero extended recurrence. Otherwise return NULL.
-const SCEVAddRecExpr *WidenIV::getWideRecurrence(Instruction *NarrowUse) {
-  if (!SE->isSCEVable(NarrowUse->getType()))
+const SCEVAddRecExpr *WidenIV::getWideRecurrence(NarrowIVDefUse DU, 
+                                                 bool &WidenBySext) {
----------------
Here and elsewhere please document the new `WidenBySext` paramenter.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1218
@@ +1217,3 @@
+  }
+  else if (ExtendKindMap[DU.NarrowDef]) {
+    WideExpr = SE->getSignExtendExpr(NarrowExpr, WideType);
----------------
clang-format please

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1409
@@ -1376,1 +1408,3 @@
 
+bool WidenIV::canWidenBySext(NarrowIVDefUse DU) {
+  return DU.NeverNegative || ExtendKindMap[DU.NarrowDef];
----------------
I'd spell it as `canWidenBySExt` (same below).

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1496
@@ -1455,2 +1495,3 @@
   pushNarrowIVUsers(OrigPhi, WidePhi);
+  ExtendKindMap[OrigPhi] = IsSigned;
 
----------------
Does `ExtendKindMap` subsume the `IsSigned` field?  If so, that field should be removed.


https://reviews.llvm.org/D24280