[NFCI][IndVars] rewriteLoopExitValues(): nowadays SCEV should not change `GEP` base pointer

[lebedev.ri]

Summary

Currently/previously, while SCEV guaranteed that it produces the same value,
the way it was produced may be illegal IR, so we have an ugly check that
the replacement is valid.

But now that the SCEV strictness wrt the pointer/integer types has been improved,
i believe this invariant is already upheld by the SCEV itself, natively.

I think we should add an assertion, wait for a week, and then, if all is good,
rip out all this checking.
Or we could just do the latter directly i guess.

This reverts commit rL127839.

[nikic]

LGTM

[lebedev.ri]

In D108043#2945574, @nikic wrote:

LGTM

Thank you for the review!
Assuming no one complains before then, i plan on rewriting this code and dropping the assertion completely in T+7days.

[bjope]

I just wrote https://bugs.llvm.org/show_bug.cgi?id=51490 which triggered this assert (using -Oz and legacy PM).

[lebedev.ri]

In D108043#2946441, @bjope wrote:

I just wrote https://bugs.llvm.org/show_bug.cgi?id=51490 which triggered this assert (using -Oz and legacy PM).

Reduced this looks like the following:

; ModuleID = 'input.ll'
source_filename = "/tmp/csmith-2231729227.c"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

@g_2168 = external global [4 x [6 x i32]], align 16
@g_1150 = external global i32*, align 8

define internal fastcc void @func_2() unnamed_addr {
lbl_2898.preheader:
  br label %lbl_2898

lbl_2898.loopexit:                                ; preds = %for.cond884
  %.lcssa = phi i32* [ %0, %for.cond884 ]
  store i32* %.lcssa, i32** @g_1150, align 8, !tbaa !0
  br label %lbl_2898

lbl_2898:                                         ; preds = %lbl_2898.loopexit, %lbl_2898.preheader
  %g_1150.promoted = load i32*, i32** @g_1150, align 8, !tbaa !0
  br label %for.cond884

for.cond884:                                      ; preds = %for.end987, %lbl_2898
  %0 = phi i32* [ getelementptr inbounds ([4 x [6 x i32]], [4 x [6 x i32]]* @g_2168, i64 0, i64 3, i64 1), %for.end987 ], [ %g_1150.promoted, %lbl_2898 ]
  %storemerge9 = phi i16 [ %add990, %for.end987 ], [ 0, %lbl_2898 ]
  %cmp886 = icmp slt i16 %storemerge9, 3
  br i1 %cmp886, label %for.body888, label %lbl_2898.loopexit

for.body888:                                      ; preds = %for.cond884
  br label %for.cond918

for.cond918:                                      ; preds = %for.body888
  br label %for.end926

for.end926:                                       ; preds = %for.cond918
  br label %for.cond936

for.cond936:                                      ; preds = %for.end926
  br label %for.end987

for.end987:                                       ; preds = %for.cond936
  %add990 = add i16 %storemerge9, 1
  br label %for.cond884
}

!0 = !{!1, !1, i64 0}
!1 = !{!"any pointer", !2, i64 0}
!2 = !{!"omnipotent char", !3, i64 0}
!3 = !{!"Simple C/C++ TBAA"}

INDVARS: Turn to unsigned comparison:   %cmp886 = icmp slt i16 %storemerge9, 3
rewriteLoopExitValues: AfterLoopVal = i32* getelementptr inbounds ([4 x [6 x i32]], [4 x [6 x i32]]* @g_2168, i64 0, i64 3, i64 1)
  LoopVal =   %0 = phi i32* [ getelementptr inbounds ([4 x [6 x i32]], [4 x [6 x i32]]* @g_2168, i64 0, i64 3, i64 1), %for.end987 ], [ %g_1150.promoted, %lbl_2898 ]
rewriteLoopExitValues: GEP rewrite bail out %0 != @g_2168
opt: /repositories/llvm-project/llvm/lib/Transforms/Utils/LoopUtils.cpp:1354: int llvm::rewriteLoopExitValues(llvm::Loop *, llvm::LoopInfo *, llvm::TargetLibraryInfo *, llvm::ScalarEvolution *, const llvm::TargetTransformInfo *, llvm::SCEVExpander &, llvm::DominatorTree *, llvm::ReplaceExitVal, SmallVector<llvm::WeakTrackingVH, 16> &): Assertion `(!ZZAssert || Phi.ValidRewrite) && "Now that the SCEV is strict wrt pointer/integer types, this " "invariant is expected to be uphold by SCEV itself."' failed.

So we are trying to rewrite %0 = phi, and manage to rewrite it as the value when coming from %for.end987 (and we must do so originally)
If we've come from %lbl_2898, then %0 is stored into global, and loaded back.
Should that obscure pointer base as far as isValidRewrite() is concerned?

[lebedev.ri]

@efriedma @mkazantsev any thoughts on just completely dropping isValidRewrite()?

[lebedev.ri]

The more aggressive fix has landed.

llvm/lib/Transforms/Utils/LoopUtils.cpp

Phi.Expansion = Rewriter.expandCodeFor(Phi.ExpansionSCEV, Phi.PN->getType(),
                                            Phi.ExpansionPoint);
 
     LLVM_DEBUG(dbgs() << "rewriteLoopExitValues: AfterLoopVal = "
                       << *(Phi.Expansion) << '\n'
                       << "  LoopVal = " << *(Phi.ExpansionPoint) << "\n");
 
     // FIXME: isValidRewrite() is a hack. it should be an assert, eventually.
     Phi.ValidRewrite = isValidRewrite(SE, Phi.ExpansionPoint, Phi.Expansion);
+    assert(Phi.ValidRewrite &&
+           "Now that the SCEV is strict wrt pointer/integer types, this "
+           "invariant is expected to be uphold by SCEV itself.");
     if (!Phi.ValidRewrite) {
       DeadInsts.push_back(Phi.Expansion);
       continue;
     }
 
 #ifndef NDEBUG
     // If we reuse an instruction from a loop which is neither L nor one of
     // its containing loops, we end up breaking LCSSA form for this loop by