[PATCH] D24280: [IndVarSimplify] Wisely choose sext or zext when widening IV

[Li Huang via llvm-commits]

lihuang marked 8 inline comments as done.
lihuang added a comment.

Add another test case where zext should not be removed.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:901
@@ +900,3 @@
+  // A map tracking the kind of extension used to widen each narrow IV 
+  // and narrow IV user.
+  // Key: pointer to a narrow IV or IV user.
----------------
Sorry for the late reply, I was working on other stuff.

Added an enum type ExtendKind.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:940
@@ -932,2 +939,3 @@
+  WidenedRecTy getExtendedOperandRecurrence(NarrowIVDefUse DU);
 
   const SCEV *getSCEVByOpCode(const SCEV *LHS, const SCEV *RHS,
----------------
thanks for the suggestion, I changed the return type to a std::pair of AddRec* and ExtendKind.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1163-1164
@@ -1148,3 +1162,4 @@
     cast<OverflowingBinaryOperator>(DU.NarrowUse);
-  if (IsSigned && OBO->hasNoSignedWrap())
+  ExtendKind ExtKind = ExtendKindMap[DU.NarrowDef];
+  if (ExtKind == SignExtended && OBO->hasNoSignedWrap())
     ExtendOperExpr = SE->getSignExtendExpr(
----------------
Actually DU.NarrowDef should be already in this map when we come to visit NarrowUse, because we visit NarrowDef before pushing NarrowUse to the queue when doing BFS. 

I added an assert in WidenIVUse to ensure this.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1196-1197
@@ -1179,6 +1195,4 @@
 /// widening it's type? In other words, can the extend be safely hoisted out of
 /// the loop with SCEV reducing the value to a recurrence on the same loop. If
-/// so, return the sign or zero extended recurrence. Otherwise return NULL.
-const SCEVAddRecExpr *WidenIV::getWideRecurrence(Instruction *NarrowUse) {
-  if (!SE->isSCEVable(NarrowUse->getType()))
-    return nullptr;
+/// so, return the extended recurrence and the kind of extension used. Otherwise
+/// return {nullptr, Unknown}.
----------------
Removed this parameter.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1502
@@ -1455,3 +1501,3 @@
   pushNarrowIVUsers(OrigPhi, WidePhi);
 
   while (!NarrowIVUsers.empty()) {
----------------
Yes it does, removed the IsSigned field from WidenIV.


https://reviews.llvm.org/D24280