[LoopUnswitch] Fix incorrect Modified status

[dstenb]

Summary

When hoisting simple values out from a loop, and an optsize attribute, a
convergent call, or an invoke instruction hindered the pass from
unswitching the loop, the pass would return an incorrect Modified
status.

This was caught using the check introduced by D80916.

[asbirlea]

If you'd be able to add reproducers in follow up patch, that would be great.

[dstenb]

Slight refactoring of the existing test case, and add test cases for the other two returns.

[dstenb]

In D86085#2221693, @asbirlea wrote:

If you'd be able to add reproducers in follow up patch, that would be great.

It was easier to create reproducers than I initially thought, so I just added those two tests to this patch also. Does this look okay?

[asbirlea]

Great, thank you!

llvm/lib/Transforms/Scalar/LoopUnswitch.cpp

bool LoopUnswitch::processCurrentLoop() {
   if (tryTrivialLoopUnswitch(Changed)) {
     return true;
   }
 
   // Do not do non-trivial unswitch while optimizing for size.
   // FIXME: Use Function::hasOptSize().
   if (OptimizeForSize ||
       LoopHeader->getParent()->hasFnAttribute(Attribute::OptimizeForSize))
-    return false;
+    return Changed;
 
   // Run through the instructions in the loop, keeping track of three things:
   //
   //  - That we do not unswitch loops containing convergent operations, as we
   //    might be making them control dependent on the unswitch value when they
   //    were not before.
   //    FIXME: This could be refined to only bail if the convergent operation is
   //    not already control-dependent on the unswitch value.
   //
   //  - That basic blocks in the loop contain invokes whose predecessor edges we
   //    cannot split.
   //
   //  - The set of guard intrinsics encountered (these are non terminator
   //    instructions that are also profitable to be unswitched).
 
   SmallVector<IntrinsicInst *, 4> Guards;
 
   for (const auto BB : CurrentLoop->blocks()) {
     for (auto &I : *BB) {
       auto *CB = dyn_cast<CallBase>(&I);
       if (!CB)
         continue;
       if (CB->isConvergent())
-        return false;
+        return Changed;
       if (auto *II = dyn_cast<InvokeInst>(&I))
         if (!II->getUnwindDest()->canSplitPredecessors())
-          return false;
+          return Changed;
       if (auto *II = dyn_cast<IntrinsicInst>(&I))
         if (II->getIntrinsicID() == Intrinsic::experimental_guard)
           Guards.push_back(II);
     }
   }
 
   for (IntrinsicInst *Guard : Guards) {
     Value *LoopCond = findLIVLoopCondition(Guard->getOperand(0), CurrentLoop,

llvm/test/Transforms/LoopUnswitch/convergent-hoist-modified.ll

+; RUN: opt -loop-unswitch -enable-new-pm=0 %s -S | FileCheck %s
+
+; When hoisting simple values out from a loop, and not being able to unswitch
+; the loop due to the convergent call, the pass would return an incorrect
+; Modified status. This was caught by the pass return status check that is
+; hidden under EXPENSIVE_CHECKS.
+
+; CHECK-LABEL: entry:
+; CHECK-NEXT: %0 = call i32 @llvm.objectsize.i32.p0i8(i8* bitcast (%struct.anon* @b to i8*), i1 false, i1 false, i1 false)
+; CHECK-NEXT: %1 = icmp uge i32 %0, 1
+; CHECK-NEXT: br label %for.cond
+
+%struct.anon = type { i16 }
+
+@b = global %struct.anon zeroinitializer, align 1
+
+; Function Attrs: nounwind
+define i16 @c() #0 {
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %cont, %entry
+  br label %for.inc
+
+for.inc:                                          ; preds = %for.cond
+  %0 = call i32 @llvm.objectsize.i32.p0i8(i8* bitcast (%struct.anon* @b to i8*), i1 false, i1 false, i1 false)
+  %1 = icmp uge i32 %0, 1
+  br i1 %1, label %cont, label %cont
+
+cont:                                             ; preds = %for.inc
+  call void @conv() convergent
+  %2 = load i16, i16* getelementptr inbounds (%struct.anon, %struct.anon* @b, i32 0, i32 0), align 1
+  br label %for.cond
+}
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare i32 @llvm.objectsize.i32.p0i8(i8*, i1 immarg, i1 immarg, i1 immarg) #1
+
+declare void @conv() convergent
+
+attributes #0 = { nounwind }
+attributes #1 = { nounwind readnone speculatable willreturn }

llvm/test/Transforms/LoopUnswitch/invoke-hoist-modified.ll

+; RUN: opt -loop-unswitch -enable-new-pm=0 %s -S | FileCheck %s
+
+; When hoisting simple values out from a loop, and not being able to unswitch
+; the loop due to the invoke instruction, the pass would return an incorrect
+; Modified status. This was caught by the pass return status check that is
+; hidden under EXPENSIVE_CHECKS.
+
+; CHECK-LABEL: for.cond:
+; CHECK-NEXT: %0 = call i32 @llvm.objectsize.i32.p0i8(i8* bitcast (%struct.anon* @b to i8*), i1 false, i1 false, i1 false)
+; CHECK-NEXT: %1 = icmp uge i32 %0, 1
+; CHECK-NEXT: br label %for.inc
+
+%struct.anon = type { i16 }
+
+@b = global %struct.anon zeroinitializer, align 1
+
+; Function Attrs: nounwind
+define i32 @c() #0 personality i32 (...)* @__CxxFrameHandler3 {
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %cont, %entry
+  br label %for.inc
+
+for.inc:                                          ; preds = %for.cond
+  %0 = call i32 @llvm.objectsize.i32.p0i8(i8* bitcast (%struct.anon* @b to i8*), i1 false, i1 false, i1 false)
+  %1 = icmp uge i32 %0, 1
+  br i1 %1, label %delete.notnull, label %delete.notnull
+
+delete.notnull:                                   ; preds = %for.inc
+  invoke void @g() to label %invoke.cont unwind label %lpad
+
+invoke.cont:                                      ; preds = %delete.notnull
+  br label %for.inc
+
+lpad:                                             ; preds = %delete.notnull
+  %cp = cleanuppad within none []
+  cleanupret from %cp unwind to caller
+
+cont:                                             ; preds = %for.inc
+  %2 = load i16, i16* getelementptr inbounds (%struct.anon, %struct.anon* @b, i32 0, i32 0), align 1
+  br label %for.cond
+}
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare i32 @llvm.objectsize.i32.p0i8(i8*, i1 immarg, i1 immarg, i1 immarg) #1
+
+declare void @g()
+
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+attributes #1 = { nounwind readnone speculatable willreturn }

llvm/test/Transforms/LoopUnswitch/optsize-hoist-modified.ll

+; RUN: opt -loop-unswitch -enable-new-pm=0 %s -S | FileCheck %s
+
+; When hoisting simple values out from a loop, and not being able to attempt to
+; non-trivally unswitch the loop, due to the optsize attribute, the pass would
+; return an incorrect Modified status. This was caught by the pass return
+; status check that is hidden under EXPENSIVE_CHECKS.
+
+; CHECK-LABEL: entry:
+; CHECK-NEXT: %0 = call i32 @llvm.objectsize.i32.p0i8(i8* bitcast (%struct.anon* @b to i8*), i1 false, i1 false, i1 false)
+; CHECK-NEXT: %1 = icmp uge i32 %0, 1
+; CHECK-NEXT: br label %for.cond
+
+%struct.anon = type { i16 }
+
+@b = global %struct.anon zeroinitializer, align 1
+
+; Function Attrs: minsize nounwind optsize
+define i16 @c() #0 {
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %cont, %entry
+  br label %for.inc
+
+for.inc:                                          ; preds = %for.cond
+  %0 = call i32 @llvm.objectsize.i32.p0i8(i8* bitcast (%struct.anon* @b to i8*), i1 false, i1 false, i1 false)
+  %1 = icmp uge i32 %0, 1
+  br i1 %1, label %cont, label %cont
+
+cont:                                             ; preds = %for.inc
+  %2 = load i16, i16* getelementptr inbounds (%struct.anon, %struct.anon* @b, i32 0, i32 0), align 1
+  br label %for.cond
+}
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare i32 @llvm.objectsize.i32.p0i8(i8*, i1 immarg, i1 immarg, i1 immarg) #1
+
+attributes #0 = { minsize nounwind optsize }
+attributes #1 = { nounwind readnone speculatable willreturn }