[IndVars] Set Changed when we delete dead instructions. PR38855

[mkazantsev]

Summary

IndVars does not set Changed flag when it eliminates dead instructions. As result,
it may make IR modifications and report that it has done nothing. It leads to inconsistent
preserved analyzes results.

[uabelho]

I'm not sure if I'm allowed to LGTM this but it does look good to me and it solves PR38855 so thumbs up.

[skatkov]

lib/Transforms/Scalar/IndVarSimplify.cpp

2494

Changed |= ...?
RecursivelyDeleteTriviallyDeadInstructions may return false indicating that it did not remove anything...

2502

should it be fixed as well? And other places?

[mkazantsev]

lib/Transforms/Scalar/IndVarSimplify.cpp

2494

Not sure if it's happening in any real case (i.e. considering how DeadInsts is formed), but OK.

2502

Maybe. It needs comprehensive analysis. I need to revisit any single place separately, let's not do it in this patch. It is to address a particular crash, however I agree that we might take a closer look into how Changed is sad.

[mkazantsev]

lib/Transforms/Scalar/IndVarSimplify.cpp

2502

Filed https://bugs.llvm.org/show_bug.cgi?id=38863 to follow-up.

[skatkov]

LGTM with follow-up handling of Changed. If you do not have cycles for handling Changed, please file a bug at least.

lib/Transforms/Scalar/IndVarSimplify.cpp

#endif
   // can be deleted in the loop below, causing the AssertingVH in the cache to
   // trigger.
   Rewriter.clear();
 
   // Now that we're done iterating through lists, clean up any instructions
   // which are now dead.
   while (!DeadInsts.empty())
     if (Instruction *Inst =
-            dyn_cast_or_null<Instruction>(DeadInsts.pop_back_val()))
+            dyn_cast_or_null<Instruction>(DeadInsts.pop_back_val())) {
+      Changed = true;
       RecursivelyDeleteTriviallyDeadInstructions(Inst, TLI);
+    }
 
   // The Rewriter may not be used from this point on.
 
   // Loop-invariant instructions in the preheader that aren't used in the
   // loop may be sunk below the loop to reduce register pressure.
   sinkUnusedInvariants(L);
 
   // rewriteFirstIterationLoopExitValues does not rely on the computation of
   // trip count and therefore can further simplify exit values in addition to
   // rewriteLoopExitValues.
   rewriteFirstIterationLoopExitValues(L);
 
   // Clean up dead instructions.
   Changed |= DeleteDeadPHIs(L->getHeader(), TLI);

test/Transforms/IndVarSimplify/pr38855.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -inline -functionattrs -indvars  < %s | FileCheck %s
+
+; Check that the invalidation happens correctly and the test does not crash.
+define void @f2() {
+; CHECK-LABEL: @f2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_COND:%.*]]
+; CHECK:       for.cond:
+; CHECK-NEXT:    br label [[FOR_COND]]
+;
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.cond, %entry
+  %a.0 = phi i32 [ 1, %entry ], [ 0, %for.cond ]
+  call void @f1(i32 %a.0)
+  br label %for.cond
+}
+
+define internal void @f1(i32 %p1) noinline {
+entry:
+  ret void
+}