[CallSiteSplitting] Do not crash when BB's terminator changes.

[fhahn]

Summary

Change doCallSiteSplitting to iterate until we reach the terminator instruction.
tryToSplitCallSite can replace BB's terminator in case BB is a successor of
itself. Then IE will be invalidated and we also have to check the current
terminator.

[fhahn]

I am not entirely sure if that's the best solution, but it should work. Maybe it would be nicer to split the pas into an analysis and transformation phase, then we would get rid of the problem.

@uabelho it would be great if you could check if this patch together with D43824 fixes the regressions you are seeing?

[junbuml]

Assuming D43824 is fixed, this seems to be an easy quick fix, but I tend to lean on separating this pass into two phases; in D43729, we also need to exit early. I'm okay to get it in first, followed by a patch to make separate phases.

[uabelho]

In D43824#1020767, @fhahn wrote:

I am not entirely sure if that's the best solution, but it should work. Maybe it would be nicer to split the pas into an analysis and transformation phase, then we would get rid of the problem.

@uabelho it would be great if you could check if this patch together with D43824 fixes the regressions you are seeing?

@fhahn: I applied D43822 together with D43824 on top of tree and that made the crash that I saw go away.

Great!

[uabelho]

@fhahn: What the status with this now?

I've reverted r325126 (Recommit r325001: [CallSiteSplitting] Support splitting of blocks with instrs before call.) downstream
to avoid the regressions I saw, but that is getting increasingly difficult now since D43729 made it into tree.

It would thus be very nice if this (and D43822 ?) could land so I could remove my local reverts.

[fhahn]

@fhahn: What the status with this now?

I think it's mostly pending review of D43822

[uabelho]

In D43824#1026929, @fhahn wrote:

@fhahn: What the status with this now?

I think it's mostly pending review of D43822

In D43824#1026929, @fhahn wrote:

@fhahn: What the status with this now?

I think it's mostly pending review of D43822

Great! I saw D43822 was accepted now so then you could submit them?

[fhahn]

Accepting in to make arc happy. Jun, I created a TODO for myself to split it up in analysis & transformation. It might take a few weeks until I get around to that though.

lib/Transforms/Scalar/CallSiteSplitting.cpp

return tryToSplitOnPredicatedArgument(CS) ||
          tryToSplitOnPHIPredicatedArgument(CS);
 }
 
 static bool doCallSiteSplitting(Function &F, TargetLibraryInfo &TLI,
                                 TargetTransformInfo &TTI) {
   bool Changed = false;
   for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE;) {
     BasicBlock &BB = *BI++;
-    for (BasicBlock::iterator II = BB.begin(), IE = BB.end(); II != IE;) {
+    auto II = BB.getFirstNonPHIOrDbg()->getIterator();
+    auto IE = BB.getTerminator()->getIterator();
+    // Iterate until we reach the terminator instruction. tryToSplitCallSite
+    // can replace BB's terminator in case BB is a successor of itself. In that
+    // case, IE will be invalidated and we also have to check the current
+    // terminator.
+    while (II != IE && &*II != BB.getTerminator()) {
       Instruction *I = &*II++;
       CallSite CS(cast<Value>(I));
       if (!CS || isa<IntrinsicInst>(I) || isInstructionTriviallyDead(I, &TLI))
         continue;
 
       Function *Callee = CS.getCalledFunction();
       if (!Callee || Callee->isDeclaration())
         continue;

test/Transforms/CallSiteSplitting/split-loop.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -callsite-splitting -simplifycfg < %s | FileCheck %s
+
+define i16 @test1() {
+; CHECK-LABEL: @test1(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[SPEC_SELECT:%.*]] = select i1 undef, i16 1, i16 0
+; CHECK-NEXT:    call void @callee(i16 0)
+; CHECK-NEXT:    br label [[FOR_COND12:%.*]]
+; CHECK:       for.cond12:
+; CHECK-NEXT:    call void @callee(i16 [[SPEC_SELECT]])
+; CHECK-NEXT:    br label [[FOR_COND12]]
+;
+entry:
+  %spec.select = select i1 undef, i16 1, i16 0
+  %tobool18 = icmp ne i16 %spec.select, 0
+  br i1 %tobool18, label %for.cond12.us, label %for.cond12
+
+for.cond12.us:
+  unreachable
+
+for.cond12:
+  call void @callee(i16 %spec.select)
+  br label %for.cond12
+}
+
+define i16 @test2() {
+; CHECK-LABEL: @test2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[S:%.*]] = select i1 undef, i16 1, i16 0
+; CHECK-NEXT:    call void @callee(i16 0)
+; CHECK-NEXT:    br label [[FOR_COND12:%.*]]
+; CHECK:       for.cond12:
+; CHECK-NEXT:    [[ADD:%.*]] = add i16 [[S]], 10
+; CHECK-NEXT:    [[ADD2:%.*]] = add i16 [[S]], 10
+; CHECK-NEXT:    call void @callee(i16 [[S]])
+; CHECK-NEXT:    br label [[FOR_COND12]]
+;
+entry:
+  %s= select i1 undef, i16 1, i16 0
+  %tobool18 = icmp ne i16 %s, 0
+  br i1 %tobool18, label %for.cond12.us, label %for.cond12
+
+for.cond12.us:
+  unreachable
+
+for.cond12:
+  call void @callee(i16 %s)
+  %add = add i16 %s, 10
+  %add2 = add i16 %s, 10
+  br label %for.cond12
+}
+
+define i16 @test3() {
+; CHECK-LABEL: @test3(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[S:%.*]] = select i1 undef, i16 1, i16 0
+; CHECK-NEXT:    call void @callee(i16 0)
+; CHECK-NEXT:    br label [[FOR_COND12:%.*]]
+; CHECK:       for.cond12:
+; CHECK-NEXT:    [[ADD:%.*]] = add i16 [[S]], 10
+; CHECK-NEXT:    [[ADD2:%.*]] = add i16 [[ADD]], 10
+; CHECK-NEXT:    br i1 undef, label [[FOR_COND12_SPLIT:%.*]], label [[EXIT:%.*]]
+; CHECK:       for.cond12.split:
+; CHECK-NEXT:    call void @callee(i16 [[S]])
+; CHECK-NEXT:    br label [[FOR_COND12]]
+; CHECK:       exit:
+; CHECK-NEXT:    ret i16 [[ADD2]]
+;
+entry:
+  %s= select i1 undef, i16 1, i16 0
+  %tobool18 = icmp ne i16 %s, 0
+  br i1 %tobool18, label %for.cond12.us, label %for.cond12
+
+for.cond12.us:
+  unreachable
+
+for.cond12:
+  call void @callee(i16 %s)
+  %add = add i16 %s, 10
+  %add2 = add i16 %add, 10
+  br i1 undef, label %for.cond12, label %exit
+
+exit:
+  ret i16 %add2
+}
+
+define internal void @callee(i16 %flag) {
+  ret void
+}