[CodeExtractor] Fix extraction of a value used only by intrinsics outside of region

[ekatz]

Summary

We should only skip lifetime and dbg intrinsics when searching for users.
Other intrinsics are legit users that can't be ignored.

Without this fix, the testcase would result in an invalid IR. memcpy will have a reference to the, now, external value (local to the extracted loop function).

Fix PR42194

[vsk]

llvm/lib/Transforms/Utils/CodeExtractor.cpp

454

Could you add a comment: // We don't model addresses with multiple start/end markers, but the markers do not need to be in the region.

455

^ then we can delete this comment about 'multiple start markers'

467

Could you add a comment: // At this point, permit debug uses outside of the region, this is fixed in \ref fixupDebugInfoPostExtraction.

llvm/test/Transforms/CodeExtractor/LoopExtractor_alloca.ll

2

It looks like -loop-extract behaves like a ModulePass. If that's right, this test should pass if you add a RUN line like: RUN: opt -debugify-each -loop-extract ... | FileCheck %s.

This would just add synthetic debug uses before -loop-extract and strip them away afterwards, the final IR should look identical (if there are no bugs w.r.t handling debug uses).

[ekatz]

llvm/lib/Transforms/Utils/CodeExtractor.cpp

467

Sure! But as \ref is a doxygen token (and this is not a doxygen comment), I offer the following:

// At this point, permit debug uses outside of the region.
// This is fixed in a later call to fixupDebugInfoPostExtraction().

llvm/test/Transforms/CodeExtractor/LoopExtractor_alloca.ll

2

Great suggestion!
Though, -debugify-each results with:

ERROR: Instruction with empty DebugLoc in function test --  %lt.cast = bitcast i32* %v1 to i8*
ERROR: Instruction with empty DebugLoc in function test --  call void @llvm.lifetime.start.p0i8(i64 -1, i8* %lt.cast)
ERROR: Instruction with empty DebugLoc in function test --  br label %loop1.loop2_crit_edge
ERROR: Instruction with empty DebugLoc in function test --  br label %exit
ERROR: Instruction with empty DebugLoc in function test.loop2 --  ret void
ERROR: Instruction with empty DebugLoc in function test.loop1 --  ret void
WARNING: Missing line 9
WARNING: Missing variable 2
WARNING: Missing variable 4
CheckModuleDebugify [Extract loops into new functions]: FAIL
CheckFunctionDebugify [Module Verifier]: PASS
CheckFunctionDebugify [Module Verifier]: PASS
CheckFunctionDebugify [Module Verifier]: PASS

This is probably a different bug that needs to be solved.

So, instead I'll just use -debugify and update the expected result to include the debug info.

[ekatz]

Update test to include debug info, and add some comments.

[vsk]

Thanks, lgtm!

llvm/test/Transforms/CodeExtractor/LoopExtractor_alloca.ll

2

The empty DebugLoc diagnostics are interesting, but unrelated to this patch. You can use -debugify-quiet to suppress the diagnostics, create a separate test for debug info, or just use the -debugify mode. Those options all sound fine to me.

[ekatz]

llvm/test/Transforms/CodeExtractor/LoopExtractor_alloca.ll

2

It won't hurt to keep the debug info, so I guess we can just leave it as it is.

llvm/lib/Transforms/Utils/CodeExtractor.cpp

 CodeExtractor::getLifetimeMarkers(const CodeExtractorAnalysisCache &CEAC,
                                   Instruction *Addr,
                                   BasicBlock *ExitBlock) const {
   LifetimeMarkerInfo Info;
 
   for (User *U : Addr->users()) {
     IntrinsicInst *IntrInst = dyn_cast<IntrinsicInst>(U);
     if (IntrInst) {
       if (IntrInst->getIntrinsicID() == Intrinsic::lifetime_start) {
         // Do not handle the case where Addr has multiple start markers.
         if (Info.LifeStart)
           return {};
         Info.LifeStart = IntrInst;
+        continue;
       }
       if (IntrInst->getIntrinsicID() == Intrinsic::lifetime_end) {
         if (Info.LifeEnd)
           return {};
         Info.LifeEnd = IntrInst;
+        continue;
       }
+      if (isa<DbgInfoIntrinsic>(IntrInst))
         continue;
     }
     // Find untracked uses of the address, bail.
     if (!definedInRegion(Blocks, U))
       return {};
   }
 
   if (!Info.LifeStart || !Info.LifeEnd)
     return {};

llvm/test/Transforms/CodeExtractor/LoopExtractor_alloca.ll

+; RUN: opt -loop-extract -S < %s | FileCheck %s
+
+; This tests 2 cases:
+; 1. loop1 should be extracted into a function, without extracting %v1 alloca.
+; 2. loop2 should be extracted into a function, with the %v2 alloca.
+;
+; This used to produce an invalid IR, where `memcpy` will have a reference to
+; the, now, external value (local to the extracted loop function).
+
+; CHECK-LABEL: @test() {
+; CHECK-NEXT: entry:
+; CHECK-NEXT:   %v1 = alloca i32
+; CHECK-NEXT:   %p1 = bitcast i32* %v1 to i8*
+; CHECK-NEXT:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 undef, i8* %p1, i64 4, i1 true)
+
+; CHECK-LABEL: @test.loop2() {
+; CHECK-NEXT: newFuncRoot:
+; CHECK-NEXT:   %v2 = alloca i32
+; CHECK-NEXT:   %p2 = bitcast i32* %v2 to i8*
+
+; CHECK-LABEL: @test.loop1(i8* %p1) {
+; CHECK-NEXT: newFuncRoot:
+; CHECK-NOT:    alloca
+
+define void @test() {
+entry:
+  %v1 = alloca i32, align 4
+  %v2 = alloca i32, align 4
+  %p1 = bitcast i32* %v1 to i8*
+  %p2 = bitcast i32* %v2 to i8*
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 undef, i8* %p1, i64 4, i1 true)
+  br label %loop1
+
+loop1:
+  call void @llvm.lifetime.start.p0i8(i64 4, i8* %p1)
+  %r1 = call i32 @foo(i8* %p1)
+  call void @llvm.lifetime.end.p0i8(i64 4, i8* %p1)
+  %cmp1 = icmp ne i32 %r1, 0
+  br i1 %cmp1, label %loop1, label %loop2
+
+loop2:
+  call void @llvm.lifetime.start.p0i8(i64 4, i8* %p2)
+  %r2 = call i32 @foo(i8* %p2)
+  call void @llvm.lifetime.end.p0i8(i64 4, i8* %p2)
+  %cmp2 = icmp ne i32 %r2, 0
+  br i1 %cmp2, label %loop2, label %exit
+
+exit:
+  ret void
+}
+
+declare i32 @foo(i8*)
+
+declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
+declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* noalias nocapture writeonly, i8* noalias nocapture readonly, i64, i1 immarg)