[IndVarSimplify] Fix Modified status for removal of overflow intrinsics

[dstenb]

Summary

When removing an overflow intrinsic the Changed status in SimplifyIndvar
was not set, leading to the IndVarSimplify pass returning an incorrect
status.

This was caught using the check introduced by D80916.

[bjope]

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp

480

I got a feeling that these eliminate functions return true/false depending on if any changes where made. So why aren't we setting Changed=true at the using side.

Otherwise I'd expect that also eliminateTrunc below should set Changed=true before returning true. Or it would be really hard to know what to expect from these functions if only some of them update Changed.

[dstenb]

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp

480

Yes, that is a good point.

I initially thought about modifying Changed in simplifyUsers() depending on if eliminateIVUser() returns true. However, that function may return true without actually making any changes, e.g. in the case of ICmpInst, SRem, and URem instructions. I don't really know why that is the case. Therefore, I thought that this patch could be kept simple, and just align with the current behavior.

(And yes, the trunc case probably needs to be handled also.)

[uabelho]

I wrote
https://bugs.llvm.org/show_bug.cgi?id=47381
about a similar problem also caused by the missing setting of Changed in SimplifyIndvar::eliminateOverflowIntrinsic.

[reames]

LGTM for the narrow fix as written + pointing out a nearby bug.

A broader fix is worth discussing, but no reason to hold back a local fix.

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp

474

JFYI, as a separate issue, this logic shouldn't be here. We should be adding the empty extracts to the DeadInst set instead and deferring deletion.

[dstenb]

I added a comment about eliminateTrunc to the commit message. I'll see if I can create a reproducer for that, and if so, I'll upload a revision for that.

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp

if (auto *EVI = dyn_cast<ExtractValueInst>(U)) {
       else {
         assert(EVI->getIndices()[0] == 0 && "Only two possibilities!");
         EVI->replaceAllUsesWith(NewResult);
       }
       ToDelete.push_back(EVI);
     }
   }
 
   for (auto *EVI : ToDelete)
     EVI->eraseFromParent();
 
   if (WO->use_empty())
     WO->eraseFromParent();
 
+  Changed = true;
   return true;
 }
 
 bool SimplifyIndvar::eliminateSaturatingIntrinsic(SaturatingInst *SI) {
   const SCEV *LHS = SE->getSCEV(SI->getLHS());
   const SCEV *RHS = SE->getSCEV(SI->getRHS());
   if (!willNotOverflow(SE, SI->getBinaryOp(), SI->isSigned(), LHS, RHS))
     return false;

llvm/test/Transforms/IndVarSimplify/eliminate-overflow-modified.ll

+; RUN: opt < %s -indvars -S -o - | FileCheck %s
+
+; When eliminating the overflow intrinsic the indvars pass would incorrectly
+; return a false Modified status. This was caught by the pass return
+; status check that is hidden under EXPENSIVE_CHECKS.
+
+; CHECK-LABEL: for.body:
+; CHECK-NEXT: %0 = phi i16 [ %1, %for.body ], [ undef, %for.body.preheader ]
+; CHECK-NEXT: %1 = add nsw i16 %0, -1
+; CHECK-NEXT: %cmp = icmp sgt i16 %1, 0
+; CHECK-NEXT:  call void @llvm.assume(i1 %cmp)
+
+; Function Attrs: nounwind
+define void @foo() #0 {
+entry:
+  %cmp1 = icmp sgt i16 undef, 0
+  br i1 %cmp1, label %for.body.preheader, label %for.end
+
+for.body.preheader:                               ; preds = %entry
+  br label %for.body
+
+for.body:                                         ; preds = %for.body.preheader, %for.body
+  %0 = phi i16 [ %2, %for.body ], [ undef, %for.body.preheader ]
+  %1 = call { i16, i1 } @llvm.sadd.with.overflow.i16(i16 %0, i16 -1)
+  %2 = extractvalue { i16, i1 } %1, 0
+  %cmp = icmp sgt i16 %2, 0
+  call void @llvm.assume(i1 %cmp)
+  br label %for.body
+
+for.end:                                          ; preds = %entry
+  ret void
+}
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare { i16, i1 } @llvm.sadd.with.overflow.i16(i16, i16) #1
+
+; Function Attrs: nounwind willreturn
+declare void @llvm.assume(i1) #2
+
+attributes #0 = { nounwind }
+attributes #1 = { nounwind readnone speculatable willreturn }
+attributes #2 = { nounwind willreturn }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 12.0.0"}