[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 ↗

(On Diff #164354)

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

2502 ↗

(On Diff #164354)

should it be fixed as well? And other places?

[mkazantsev]

lib/Transforms/Scalar/IndVarSimplify.cpp

2494 ↗

(On Diff #164354)

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

2502 ↗

(On Diff #164354)

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 ↗

(On Diff #164354)

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.

llvm/trunk/lib/Transforms/Scalar/IndVarSimplify.cpp

#endif
   // 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()))
-      RecursivelyDeleteTriviallyDeadInstructions(Inst, TLI);
+      Changed |= 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

llvm/trunk/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
+}