This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ekatz on Apr 23 2020, 12:34 PM.

Details

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

Diff Detail

Event Timeline

ekatz created this revision.Apr 23 2020, 12:34 PM
vsk added inline comments.Apr 23 2020, 1:58 PM
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 marked 4 inline comments as done.Apr 24 2020, 12:33 AM
ekatz added inline comments.
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 updated this revision to Diff 259823.Apr 24 2020, 12:40 AM

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

vsk accepted this revision.Apr 24 2020, 9:18 AM

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.

This revision is now accepted and ready to land.Apr 24 2020, 9:18 AM
ekatz marked an inline comment as done.Apr 24 2020, 10:16 AM
ekatz added inline comments.
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.

This revision was automatically updated to reflect the committed changes.