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

[Sanjoy Das via llvm-commits]

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

Hi Li,

This is looking fairly close to ready.  I mostly have some nitpicky comments inline.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:904
@@ +903,3 @@
+  // Value: the kind of extension used to widen this Instruction.
+  DenseMap<Instruction *, ExtendKind> ExtendKindMap;
+
----------------
Make the domain of this map an `AssertingVH` to guard against use-after-free bugs.

================
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(
----------------
lihuang wrote:
> 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.
> 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.

Yes, but I'd rather have the assert automatically trigger anything you access `ExtendKindMap`.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1201
@@ -1185,1 +1200,3 @@
+  if (!SE->isSCEVable(DU.NarrowUse->getType()))
+    return std::make_pair(nullptr, SignExtended);
 
----------------
s/`SignExtended`/`Unknown`/

Not compulsory to change: you can use `{A, B}` instead of `std::make_pair(A, B)`.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1325
@@ -1291,2 +1324,3 @@
   // Our raison d'etre! Eliminate sign and zero extension.
-  if (IsSigned ? isa<SExtInst>(DU.NarrowUse) : isa<ZExtInst>(DU.NarrowUse)) {
+  if ((isa<SExtInst>(DU.NarrowUse) && canWidenBySExt(DU)) ||
+      (isa<ZExtInst>(DU.NarrowUse) && canWidenByZExt(DU))) {
----------------
Have you considered putting `canWidenBySExt` and `canWidenByZExt` as lambdas here?  That will make things more readable I think.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1367
@@ -1332,2 +1366,3 @@
     WideAddRec = getExtendedOperandRecurrence(DU);
 
+  if (!WideAddRec.first) {
----------------
Add an `assert((WideAddRec.first == nullptr) == (.second == Unknown))`.


https://reviews.llvm.org/D24280